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

Refactor config keys #351

Closed
alerque opened this issue May 7, 2021 · 1 comment
Closed

Refactor config keys #351

alerque opened this issue May 7, 2021 · 1 comment
Labels
enhancement New feature or request

Comments

@alerque
Copy link
Contributor

alerque commented May 7, 2021

While working on #350 I had a couple observations that would require app changes but might be worth doing before another version tag comes along:

# Database.
[db]

The felt need to comment [db] to mean Database suggests that this section header should just be [database]. I believe in comments, but not comments to explain things that could have been obvious.

admin_username = "listmonk"
admin_password = "listmonk"

Shouldn't there be an [admin] section with username and password keys? Partitioning admin settings separately from the main [app] settings would probably lend itself to future user authentication overhauls.

Mentioned elsewhere, apparently some keys like data_dir are processed at the top level outside any section heading. This is confusing and not documented. Most TOML settings parsers I'm familiar with keep everything under at least one section key — ­or else explicitly use a [globals], [listmonk] or some other top level heading.

@knadh
Copy link
Owner

knadh commented May 8, 2021

[database] would've been better, yep, but I don't think it's worth changing and introducing a breaking change to all existing installations. [db] may be mildly annoying, but thankfully, it's neither a functional impediment nor a deal breaker. You look at it once when setting up the config for first time and probably never again after that.

admin_username, admin_password was meant to be a temporary setup before listmonk got a proper user + auth system, but that being a significant amount of effort (that doesn't outright benefit the core functionality), it's taken a backseat. This can go away or be refactored when a proper auth system is considered. In the meanwhile, I don't think should be touched to introduce a breaking change.

Mentioned elsewhere, apparently some keys like data_dir are processed at the top level outside any section heading. This is confusing and not documented. Most TOML settings parsers I'm familiar with keep everything under at least one section key — ­or else explicitly use a [globals], [listmonk] or some other top level heading.

+1. This behaviour is because the command line flags are merged as-is into the config keys picked up from the file automatically, and the the flags were not intended to be used from the config. These keys can indeed be added to the config under [app] and be documented.

@knadh knadh added the enhancement New feature or request label May 8, 2021
@knadh knadh closed this as completed Apr 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants