Convert more operator configuration to CLI options #351
Conversation
openshift-ci-robot
added
the
size/L
openshift-ci-robot
added
the
approved
ironcladlou
force-pushed
the
options-refactoring
branch
2 times, most recently
from
7b8194b
to
bec7ae5
Compare
openshift-ci-robot
added
size/M
cmd/ingress-operator/start.go
Outdated
| cmd.Flags().StringVarP(&options.MetricsListenAddr, "metrics-listen-addr", "", ":6000", "metrics endpoint listen address") | ||
| cmd.Flags().StringVarP(&options.OperatorNamespace, "namespace", "n", manifests.DefaultOperatorNamespace, "namespace the operator is deployed to") | ||
| cmd.Flags().StringVarP(&options.IngressControllerImage, "image", "i", "", "image of the ingress controller the operator will manage (required)") | ||
| cmd.Flags().StringVarP(&options.ReleaseVersion, "release-version", "", statuscontroller.UnknownVersionValue, "the release version the operator should converge to") |
There was a problem hiding this comment.
An alternative approach, which we use in the router, is to read the environment variable to get a default value, and use a hard-coded value as the default if the environment variable is not set.
There was a problem hiding this comment.
Isn't this already what's effectively happening, since the env var is used for the CLI options in the deployment YAML?
I also went ahead and made this and most of the other options required with defaults.
There was a problem hiding this comment.
Isn't this already what's effectively happening, since the env var is used for the CLI options in the deployment YAML?
Sort of, but if you used os.Getenv in the code, then the deployment wouldn't need to translate environment variables into command-line parameters, which could be convenient for people writing their own deployment definitions or running the operator directly (if such people exist). Anyway, it isn't important; just a suggestion.
There was a problem hiding this comment.
Driving this via env variables would seem natural to me too. But, not important for now.
Convert some environment variable config to CLI options for consistency and because they're more discoverable and easier to document.
ironcladlou
force-pushed
the
options-refactoring
branch
from
bec7ae5
to
dead13f
Compare
openshift-ci-robot
added
size/L
|
/retest |
|
/lgtm |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
Something's legit broken in my latest patch, I need to fix it. /lgtm cancel |
openshift-ci-robot
added
size/M
|
/retest |
|
/test all @Miciah PTAL, clearing out the PR queue |
|
/retest |
|
@Miciah please re-tag |
|
/lgtm |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: frobware, Miciah 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 |
Convert some environment variable config to CLI options for consistency
and because they're more discoverable and easier to document.
Depends on #347.