-
Notifications
You must be signed in to change notification settings - Fork 10
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
allows controller to be configured via environment variable #31
allows controller to be configured via environment variable #31
Conversation
Welcome @Peac36! |
let's document this clearly and make sure we are consistent, IMHO flags should always be authoritative , since they are discoverable , environment variables are harder to find .. @mneverov your thoughts? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to use github.com/jessevdk/go-flags.
It has a succinct syntax, handles errors properly, and arguments have priority over environment variables.
ec88908
to
1ad312e
Compare
8eb02db
to
6399339
Compare
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mneverov, Peac36 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
c := logsapi.NewLoggingConfiguration() | ||
logsapi.AddGoFlags(c, flag.CommandLine) | ||
flag.Parse() | ||
|
||
conf := config{EnableLeaderElection: true} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not familiar with this framework, does the library sets this to false if the flag is set and ignores it if the flag is not set @mneverov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed that by default the controller should run in leader election mode. The go-flags
library does not have default values for boolean flags and suggests to use reverted flags like --disable-leader-election
which will be on by default and can be overwritten.
To workaround this and provide the same flag semantics as in #20 @Peac36 added the line above such that if --enable-leader-election
flag is not provided the controller will still run in leader election mode (again as before).
To disable leader election one can use --enable-leader-election=false
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense
/lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the new library differentiate between flag not set and set to false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When --enable-leader-election
or --enable-leader-election=true
are provided as well as when the flag is not provided at all the controller will start in leader election mode.
Disabling leader election is possible via --enable-leader-election=false
Issue: #11