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

admin: Add --defaults options #13

Merged
merged 1 commit into from Mar 11, 2024
Merged

admin: Add --defaults options #13

merged 1 commit into from Mar 11, 2024

Conversation

uristdwarf
Copy link
Collaborator

Previous iteration of the configuration dumping command did not allow dumping default values of services, since default values are set by the caller to the cfg_get* functions, thus there being no single truth to these option values or even names.

This commit adds maps of the currently known configuration options, and the dump-config command uses these to figure out what are the defaults for each service.

This is done with best-effort, because this list may become outdated as time goes on and new options are added. Until we have a single source of truth for these options, this will have to do.

Previous iteration of the configuration dumping command did not allow
dumping default values of services, since default values are set by the
caller to the cfg_get* functions, thus there being no single truth to
these option values or even names.

This commit adds maps of the currently known configuration options, and
the dump-config command uses these to figure out what are the defaults
for each service.

This is done with best-effort, because this list may become outdated as
time goes on and new options are added. Until we have a single source of
truth for these options, this will have to do.
@uristdwarf uristdwarf changed the base branch from main to dev February 29, 2024 11:55
Copy link
Contributor

@aNeutrino aNeutrino left a comment

Choose a reason for hiding this comment

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

LGTM

src/admin/dump_config_command.cc Show resolved Hide resolved
Copy link
Contributor

@lgsilva3087 lgsilva3087 left a comment

Choose a reason for hiding this comment

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

I understand this is a quick fix.
Please take into consideration that it introduces another place to update every time a default value changes.
Please, create the task to implement my proposal:

- class Option;
- class Configuration; //(singleton, map of Options)
- only access function cfg_get_* inside the previous classes.
- a function for each module to initialize the Options with default values.

@lgsilva3087 lgsilva3087 merged commit f06843e into dev Mar 11, 2024
2 checks passed
@lgsilva3087 lgsilva3087 deleted the cfg-defaults-new branch March 11, 2024 15:07
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.

None yet

3 participants