Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support for multiple configuration files in a directory #585

Merged
merged 3 commits into from
Apr 4, 2022
Merged

Support for multiple configuration files in a directory #585

merged 3 commits into from
Apr 4, 2022

Conversation

juhoinkinen
Copy link
Member

@juhoinkinen juhoinkinen commented Mar 31, 2022 •
edited by osma
Loading

Adds support for a project configuration directory. Closes #584.

Checks for duplicated project IDs across all configuration files in the configuration directory, and raises ConfigurationException if any are found.

For now I chose projects.d as the name for the default configuration directory, because it is nicely in line with the projects.cfg and projects.toml names for the default configuration files. Downside is that this name can be confusing: one could use "projects directory" to refer to the (projects) configuration directory (projects.d) or the projects data directory (data/projects). Or even to the parent directory containing projects.cfg and data/ (in Docker instances I have been using /annif-projects directory for this).

I open PR as draft now; this probably needs some more tests etc.

@juhoinkinen juhoinkinen added this to the 0.58 milestone Mar 31, 2022
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 6 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@codecov
Copy link

codecov bot commented Mar 31, 2022 •
edited
Loading

Codecov Report

Merging #585 (1883de8) into master (aff0411) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #585   +/-   ##
=======================================
  Coverage   99.47%   99.48%           
=======================================
  Files          84       84           
  Lines        5568     5615   +47     
=======================================
+ Hits         5539     5586   +47     
  Misses         29       29
Impacted Files Coverage Δ
annif/cli.py 99.63% <100.00%> (ø)
annif/config.py 100.00% <100.00%> (ø)
annif/default_config.py 100.00% <100.00%> (ø)
annif/registry.py 100.00% <100.00%> (ø)
tests/test_cli.py 100.00% <100.00%> (ø)
tests/test_config.py 100.00% <100.00%> (ø)
tests/test_project.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aff0411...1883de8. Read the comment docs.

@juhoinkinen
Copy link
Member Author

One reason to use the projects.d name instead of conf, config etc. is that the latter names make to think about some general configuration not necessarily about projects (like log file location, parallelization settings... although those are not existing in Annif now, maybe some day some like those could be).

I don't see anything to add to this now, @osma can you take a look?

@juhoinkinen juhoinkinen marked this pull request as ready for review April 1, 2022 07:44
osma
osma approved these changes Apr 1, 2022
View reviewed changes
Copy link
Member

@osma osma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good!
I tested this briefly and couldn't make it not work ;)
Great work!

@osma
Copy link
Member

osma commented Apr 1, 2022

I edited the opening post slightly ("closing #584" -> "closes #584") so that GitHub recognizes the relationship and will automatically close the issue when this PR is merged.

@juhoinkinen juhoinkinen merged commit 1d33ed7 into master Apr 4, 2022
@juhoinkinen juhoinkinen deleted the issue584-support-for-multiple-configuration-files-in-a-directory branch April 4, 2022 05:57
@juhoinkinen juhoinkinen mentioned this pull request Nov 21, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@osma osma osma approved these changes

Assignees
No one assigned
Labels
DVC enhancement
Projects
None yet
Milestone
0.58
Development

Successfully merging this pull request may close these issues.

Support for multiple configuration files in a directory
2 participants
@juhoinkinen @osma