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

Cmd line flag -c for config location #108

Merged
merged 1 commit into from
Sep 21, 2018
Merged

Cmd line flag -c for config location #108

merged 1 commit into from
Sep 21, 2018

Conversation

stblassitude
Copy link
Contributor

Add command line parsing and a flag -c to specify where the config
file should be loaded from.

@coveralls
Copy link

coveralls commented Aug 21, 2018

Coverage Status

Coverage remained the same at 93.429% when pulling 2c834c0 on stblassitude:cmdline into d66ccff on joohoi:master.

@Ajedi32
Copy link
Contributor

Ajedi32 commented Aug 22, 2018

Fixes #96

Copy link
Owner

@joohoi joohoi left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! It looks good, I only have a single concern here;

The default value is ./config.cfg, making it always being set pretty much. Previously acme-dns defaulted to checking if /etc/acme-dns/config.cfg existed, and only after then ./config.cfg. In hindsight this was a unintuitive and borderline stupid move. However I think that we need to keep the previous default behavior, as that's what people currently running the software have become to expect.

This being the fact, I'm asking if we could change the default value to an empty string, checking if the string is set, and only then overriding the old behavior.

main.go Outdated
@@ -18,14 +19,16 @@ import (
func main() {
// Created files are not world writable
syscall.Umask(0077)
configPtr := flag.String("c", "./config.cfg", "config file location")
Copy link
Owner

Choose a reason for hiding this comment

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

Could we make this to default to an empty string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to make that change, but I’m traveling at the moment and will only be able to do it next week.

Add command line parsing and a flag `-c` to specify where the config
file should be loaded from.
@stblassitude
Copy link
Contributor Author

I've updated the PR: if specified, the arguments' path is used, otherwise /etc/acme-dns/config.cfg, finally ./config.cfg.

@joohoi
Copy link
Owner

joohoi commented Sep 21, 2018

LGTM! Thanks for working on this!

@joohoi joohoi merged commit db2a6bc into joohoi:master Sep 21, 2018
jacobmyers-codeninja pushed a commit to jacobmyers-codeninja/acme-dns that referenced this pull request Sep 30, 2020
Add command line parsing and a flag `-c` to specify where the config
file should be loaded from.
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.

4 participants