🌱 Add more ctrl options flags #1725
Conversation
metal3-io-bot
added
the
size/M
|
/test metal3-bmo-e2e-test-pull |
|
/retest |
main.go
Outdated
| Cache: cache.Options{ | ||
| ByObject: secretutils.AddSecretSelector(nil), | ||
| DefaultNamespaces: watchNamespaces, | ||
| }, | ||
| } | ||
|
|
||
| if leaseDuration != "" { | ||
| dur, err := time.ParseDuration(leaseDuration) |
There was a problem hiding this comment.
Looking at the defaults, it's tempting to skip the parsing and make the command line argument -lease-duration-seconds. Nobody is going to want a duration in years or even hours. It could be a float if they really want to get down to the milliseconds.
There was a problem hiding this comment.
If we have leaseDurationSeconds, it'll create weird temporaries like:
leaseDurationSecondsDuration := time.Duration(leaseDurationSeconds) * time.Second
ctrlOptions.LeaseDuration = &leaseDurationSecondsDurationBut more annoyingly, we'll have to parse the env vars to ints. So this tidy line:
flag.StringVar(&renewDeadline, "renew-deadline", os.Getenv("RENEW_DEADLINE"), "Golang duration, e.g. '26s'")would become something more involved because os.Getenv returns a string but the default must be a number.
There was a problem hiding this comment.
it'll create weird temporaries
TBH I would take that one extra line of code over complicated user documentation.
we'll have to parse the env vars to ints
flag.IntVar() is only a convenience. If you wanted to keep it as StringVar and continue to do the parsing to int/float here yourself I don't think there's anything stopping you.
There was a problem hiding this comment.
I switched it to using seconds, left it as a StringVar and added strconv.ParseInt.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: zaneb 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 |
metal3-io-bot
added
the
approved
|
@honza did you forget to push your local changes? |
Nope. But I did push them to the wrong branch... 😞 |
We add a few command-line flags to the baremetal-operator binary to
configure more ctrl.Options{}.
Signed-off-by: Honza Pokorny <honza@redhat.com>
|
/lgtm |
|
/test metal3-bmo-e2e-test-pull |
We add a few command-line flags to the baremetal-operator binary to configure more
ctrl.Options{}.