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.

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

Add generic Zino config file #249

Merged
merged 24 commits into from
Jul 5, 2024

Conversation

johannaengland
Copy link
Contributor

@johannaengland johannaengland commented Jun 10, 2024

Closes #224.

A few notes for discussion/information:

New questions that popped up during the fixing/writing tests:

  • is it enough validation or does any other field need to be further validated?

@johannaengland johannaengland added help wanted Extra attention is needed discussion labels Jun 10, 2024
@johannaengland johannaengland self-assigned this Jun 10, 2024
Copy link

codecov bot commented Jun 10, 2024

Codecov Report

Attention: Patch coverage is 95.06173% with 4 lines in your changes missing coverage. Please review.

Project coverage is 98.85%. Comparing base (28d18e6) to head (d32ac29).
Report is 20 commits behind head on master.

Files Patch % Lines
src/zino/zino.py 88.24% 2 Missing ⚠️
src/zino/config/__init__.py 94.74% 1 Missing ⚠️
src/zino/getuptime.py 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #249      +/-   ##
==========================================
- Coverage   98.89%   98.85%   -0.03%     
==========================================
  Files          51       52       +1     
  Lines        6906     6977      +71     
==========================================
+ Hits         6829     6897      +68     
- Misses         77       80       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@johannaengland johannaengland force-pushed the add-config-file branch 2 times, most recently from a0e018c to 1d4a185 Compare June 10, 2024 10:31
Copy link

sonarcloud bot commented Jun 10, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@hmpf
Copy link
Contributor

hmpf commented Jun 10, 2024

should we require a zino.toml file to exist? or is it optional and otherwise we simply use the default values?

If the zino.toml file is used to do the job of polldevs.cf there should be an error on startup if the polldevs-section is empty.

@hmpf
Copy link
Contributor

hmpf commented Jun 10, 2024

It can be nice to have an example zino.toml with all the defaults explicitly set and commented out. This is what postgres does.

@runborg
Copy link

runborg commented Jun 10, 2024

from the config file: zino_secrets , persist_file and pollfile are all references to files, be more consistent on the names used.. e.g. secrets_file, persistence_file and device_file or something like that

it could also be beneficial to add options to categories, e.g.

[persistence]
file = "polldevs.cf"
interval = 5

@runborg
Copy link

runborg commented Jun 10, 2024

should we require a zino.toml file to exist? or is it optional and otherwise we simply use the default values?

i think everything in zino.toml should have it's default fallback in code-defaults. and those should be mandatory to have.
But it would be great to have this file created with all default values documented so the user can uncomment and change it.

If the zino.toml file is used to do the job of polldevs.cf there should be an error on startup if the polldevs-section is empty.

I do not think it should be redundant options in polldevs.cf and zino.toml.. and everythin in polldevs.cf should be directly connected to one or more devices in the polling cycle.

@hmpf
Copy link
Contributor

hmpf commented Jun 11, 2024

Also, if multiple config files, all should use the same format unless one of them only contains one string.

pyproject.toml Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
zino.toml.example Outdated Show resolved Hide resolved
Copy link
Member

@lunkwill42 lunkwill42 left a comment

Choose a reason for hiding this comment

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

You're on the right track here, but I believe it should still be possible to run Zino without having a config-file. It should just run off its default values. I might not have been too specific about this in the original issue.

A common strategy for this, used in other projects of ours, is just to define the default config internally (either as a raw representation of the config file contents, or as a default version of the dict structure the file is parsed to), and let an actual config file read operation overwrite those defaults.

(I did put some inline comments, but they must be read in light of this overall comment)

tests/conftest.py Outdated Show resolved Hide resolved
src/zino/zino.py Outdated Show resolved Hide resolved
tests/zino_test.py Outdated Show resolved Hide resolved
@johannaengland
Copy link
Contributor Author

For documentation purposes:
Old zino.cf variable names to new zino.toml variable names:

  • old_events -> archiving.old_events_dir
  • zino_secrets -> authentication.file
  • persist_file -> persistence.file
  • persist_period -> persistence.period
  • pollfile -> polling.file
  • conf_check -> polling.period

@johannaengland johannaengland force-pushed the add-config-file branch 4 times, most recently from 587687b to 30a488f Compare July 2, 2024 14:13
@runborg
Copy link

runborg commented Jul 2, 2024

As a reminder from our meeting last week:
If a configuration file does not exist zino should load with its internal defaults
If a configuration file is specified as a cli parameter ensure that the file exists and is a valid configuration, if the specified configuration file does not exist or includes errors zino should not start.

When loading the configuration file it should extend the internal default values definded in zino.
Ensure that the configuration only includes known and valid configuration options, if the user have defined a key with a illegal value or if a unknown key is defined zino should not start.

README.md Show resolved Hide resolved
src/zino/zino.py Outdated Show resolved Hide resolved
src/zino/zino.py Outdated Show resolved Hide resolved
Copy link
Member

@lunkwill42 lunkwill42 left a comment

Choose a reason for hiding this comment

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

Let's do this :)

@lunkwill42 lunkwill42 merged commit fc828ba into Uninett:master Jul 5, 2024
7 of 9 checks passed
@lunkwill42
Copy link
Member

Oh well, playing fast and loose this Friday afternoon... @johannaengland left the office and I forgot there was a fixup! commit here :-D

@johannaengland johannaengland deleted the add-config-file branch July 8, 2024 07:22
@johannaengland johannaengland mentioned this pull request Jul 25, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a generic Zino configuration file
5 participants