Move the configuration file handling code into a separate module #385
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Configuration handling will be slowly moved over to this SydentConfig class and away from the rest of the code.
Azrenbeth
force-pushed
the
azren/move_config_code_out
branch
from
e8d9964 to
bdaffc6
Compare
added 13 commits
September 8, 2021 10:57
The SydentEd25519 class is no longer needed as all it did was handle the signingkey config option before (which is now done in CryptoConfig) In the current setup, if no key is present a new one is generated and saved back to file. This is a special case for the crypto config and so the line `return self.crypto.save_key` isn't more general. In the future this might not be needed (e.g. by having sydent only do key generation when it first creates a config file for you, rather than everytime one is not found) and so this special casing can be removed.
test_jinja_templates used `("email", "email.verification_template")`
for this argument, however this config option isn't mentioned
anywhere else in the code (or even in the test's config) so it's ok
to remove it
- Move config file and dict handling over to SydentConfig - Alter all places where Sydent object is constructed - Remove parse_cfg_bool and set_from_comma_set_string from sydent.py as these functions were only used for config parsing - Remove save_config from sydent.py as this can now be done from SydentConfig
Azrenbeth
force-pushed
the
azren/move_config_code_out
branch
from
bdaffc6 to
37b928b
Compare
richvdh
changed the title
richvdh
requested changes
Sep 9, 2021
added 5 commits
September 10, 2021 10:32
- Fix typo in changelog - Remove ... between params and return in docstrings - Update copyright year - Remove conditional import for type checking if conditional not needed - Use fallback for email templates instead of has_option - Fix typo in blank server.name warning - Add template path used to template path not found warning - Set values for prometheus and sentry settings even when not enabled - Use fallback for verify_response_template instead of has_option - Set value for internal_bind_address even when API not enabled - Remove random capitilisation in ca_cert_file - Add type hint to ClientTLSOptionsFactory constructor - Add 'is deprecated' comments near old template usages - Move terms_path config option read outside of try:
added 2 commits
September 10, 2021 11:37
Apparently only have one line if it's between licence and imports!
richvdh
reviewed
Sep 10, 2021
sydent/config/general.py
Outdated
|
|
||
| self.sentry_enabled = cfg.has_option("general", "sentry_dsn") | ||
| if self.sentry_enabled: | ||
| self.sentry_dsn = cfg.get("general", "sentry_dsn") |
There was a problem hiding this comment.
Would it not be better to leave sentry_enabled in as a config setting?
you mean, as a property in this class, as opposed to a setting in the config file? It just feels a bit redundant to me - why have sentry_enabled as well as sentry_dsn?
As to consistency - I'd probably argue we get rid of prometheus_enabled and internal_api_enabled too.
Anyway, it's not a big deal, let's leave it for now.
(The eventual aim of the XYZ_enabled options being to have
enabledsettings in the config if/when it gets migrated to YAML)
I'm not entirely convinced we should migrate to YAML, and I'm also not convinced that we need explicit enabled settings :)
| if self.prometheus_enabled: | ||
| self.prometheus_port = cfg.getint("general", "prometheus_port") | ||
| self.prometheus_addr = cfg.get("general", "prometheus_addr") | ||
| self.prometheus_port = cfg.getint("general", "prometheus_port", fallback=None) |
There was a problem hiding this comment.
to confirm: have you checked that getint is happy with fallback=None as opposed to an integer?
There was a problem hiding this comment.
added 2 commits
September 10, 2021 13:45
- `parse_config` now returns bool - Add BaseConfig class - Fix typo - Update licence
added 2 commits
September 13, 2021 11:35
- Update remaining parse-configs to return bool - Add in some type hints - Parse IPs into lists not set (just needs to be Iterable) - Add missing import to __init__.py
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Seperates the config parsing from the rest of the code
Advantages of parsing the config before doing anything else:
ConfigParser.get()from the rest of the codeAll of the parsing should be exactly the same as it was before, but just done earlier
A lot of the changes are one of these two forms:
self.sydent.cfg.get("section","option")=>self.sydent.config.section.optionself.sydent.option=>self.sydent.config.general.optionSydent.get_branded_template()no longer has adeprecated_template_nameargumentCan be reviewed commit to commit