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 generate-config script and document config options #404

Open
wants to merge 16 commits into
base: azren/dict_not_configparser
Choose a base branch
from

Conversation

Azrenbeth
Copy link
Contributor

@Azrenbeth Azrenbeth commented Sep 17, 2021

Builds on #402. Fixes #101.

Stop using CONFIG_DEFAULTS dictionary

  • Replace in code with config.get(option, default_value)
  • List of available options (and defaults) can now be found in new documentation file (docs/configuration.md)

Stop generating config file on first run of Sydent. For reasons brought up in #401. This will not affect people with existing config files, only new installers.

  • Add a generate-config script that creates a minimal config.
  • Add an example config file (docs/example_config.conf)

Not sure if I did the correct thing:

  • currently, the environment variables that Sydent reads (e.g. SYDENT_PID_FILE) are used to define some default config options ONLY on the first time sydent is run (or if you edit the config file to delete the things that Sydent adds automaticall)
  • Now, those defaults are available to be set using the generate-config script (e.g. the --pid-file argument) with the environment variables taking precedence. But if the user's config file does not have a value set, then the environment variable is used.
  • As an example:
    1. I run generate-config with no arguments
    2. I run sydent which uses sydent.pid to store the process ID
    3. I set SYDENT_PID_FILE to somefile.pid
    4. I run sydent which uses somefile.pid to store the process ID

An alternative would have been to require those values to be set by the config file.

@Azrenbeth Azrenbeth force-pushed the azren/gnerate_config_and_document branch from d47be64 to 7d29f6c Compare September 20, 2021 15:07
@Azrenbeth Azrenbeth changed the base branch from main to azren/dict_not_configparser September 20, 2021 15:10
@Azrenbeth Azrenbeth marked this pull request as ready for review September 20, 2021 15:14
@DMRobertson DMRobertson requested a review from a team February 1, 2022 19:40
@babolivier
Copy link
Contributor

Note that since @Azrenbeth has finished his internship with us it's likely someone else will have to take this over.

@clokep clokep removed the request for review from a team February 3, 2022 13:38
@clokep
Copy link
Member

clokep commented Feb 3, 2022

I'm removing review on this since it depends on #402, which is a sizeable PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants