feat: Enhancement for namespace installation mode configuration #1939
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #1939 +/- ##
========================================
Coverage ? 10.7%
========================================
Files ? 35
Lines ? 25033
Branches ? 0
========================================
Hits ? 2681
Misses ? 22012
Partials ? 340Continue to review full report at Codecov.
|
jessesuen
requested changes
Jan 11, 2020
alexec
reviewed
Jan 13, 2020
alexec
reviewed
Jan 13, 2020
manifests/namespace-install/overlays/workflow-controller-deployment.yaml
Outdated
Show resolved
Hide resolved
|
@alexec - could you review again? |
alexec
reviewed
Jan 13, 2020
cmd/workflow-controller/main.go
Outdated
| @@ -96,6 +119,12 @@ func NewRootCommand() *cobra.Command { | |||
| command.Flags().IntVar(&glogLevel, "gloglevel", 0, "Set the glog logging level") | |||
| command.Flags().IntVar(&workflowWorkers, "workflow-workers", 8, "Number of workflow workers") | |||
| command.Flags().IntVar(&podWorkers, "pod-workers", 8, "Number of pod workers") | |||
| namespacedMode, err := strconv.ParseBool(os.Getenv("NAMESPACED")) | |||
There was a problem hiding this comment.
os.Getenv("NAMESPACED") == "true"
alexec
approved these changes
Jan 13, 2020
jessesuen
reviewed
Jan 14, 2020
There was a problem hiding this comment.
Conditional approval:
I don't think we should use an environment variable to configure this because:
- env vars it's not self-documenting from the CLI. This magic env var was added, but now it needs to be documented.
- the controller has never used environment variables as a configuration, so we shouldn't start now
- we should avoid two different ways of doing the same thing
- core functionality like this should be promoted as a CLI flag, since an environment variable makes it ambiguous if it took effect or even relevant to the release (e.g. a user might try to set
NAMESPACED=truein an older release and wonder why it's not working).
Can change this so that we only use CLI flag instead of environment variable?
jessesuen
approved these changes
Jan 14, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is the implementation for proposal #1938, with this change,
workflow-controlleradds 2 optional args--namespacedand--namespace.This deprecates the dependency of
namespaceconfiguration in the configmap for namespace only mode installation.