osm install --set arguments should override default flag values Go
Bug description:
If the --set
flag is used for osm install
but the value being passed in already has a default since it has its own flag for install, the argument passed to --set
should override the default.
More details: https://github.com/openservicemesh/osm/pull/2784/files#r591035725 Affected area (please mark with X where applicable): - Install [X] - SMI Traffic Access Policy [ ] - SMI Traffic Specs Policy [ ] - SMI Traffic Split Policy [ ] - Permissive Traffic Policy [ ] - Ingress [ ] - Egress [ ] - Envoy Control Plane [ ] - CLI Tool [ ] - Metrics [ ] - Certificate Management [ ] - Sidecar Injection [ ] - Logging [ ] - Debugging [ ] - Tests [ ] - CI System [ ]
Expected behavior:
osm install --set="OpenServiceMesh.enablePrivilegedInitContainer=true" --set="OpenServiceMesh.envoyLogLevel=info"
The above command should override the envoyLogLevel
Steps to reproduce the bug (as precisely as possible):
osm install --set="OpenServiceMesh.enablePrivilegedInitContainer=true" --set="OpenServiceMesh.envoyLogLevel=info"
How was OSM installed?: osm CLI Anything else we need to know?:
Environment:
- OSM version (use osm version
): 0.8.0
- Kubernetes version (use kubectl version
): 1.18
- Size of cluster (number of worker nodes in the cluster): 2
- Others:
5 Answer:
This behavior is intended, though I understand it's not ideal.
The help text for the flag does address this behavior somewhat:
--set stringArray Set arbitrary chart values not settable by another flag...
--set
doesn't override the other flags because currently the validation for those values is tied to the CLI flag. The current behavior ensures that validation can't be bypassed using --set
.
--set
doesn't override the other flags because currently the validation for those values is tied to the CLI flag. The current behavior ensures that validation can't be bypassed using--set
.
This should be resolvable with a bit of refactoring, right? We'd just have to "merge" the set values first, then validate, then resolve.
Yeah, that should work. My only concern is the error messages will probably say something like OpenServiceMesh.envoyLogLevel not valid...
if a user does osm install --envoy-log-level=invalid
. Usually the mapping between osm install
flags and values is easy enough to figure out, but I could see that being unclear.
Can we make the usage of chart param specific flags consistent with either using the CLI flags or using --set
and ensuring we can validate every param passed to --set
? There needs to be a clear guidance on when to use a CLI flag vs --set
. Otherwise this is going to introduce more inconsistency in the code and make it hard to reason. I don't believe we should be providing multiple ways to set the same chart param, its just harder to maintain and reason.
Commit bff18e72c702478ee588433547d80b17240d8e83 removed custom CLI flags for the install commands in favor of using --set
, so this issue should be resolved.
Read next
- patternProperties interferes with properties - jsonschema-rs
- Bootstrap logo (fa-bootstrap) is out of date - JavaScript Font-Awesome
- Serilog Elasticsearch index name does not get created - serilog-sinks-elasticsearch
- Unable to hide my drawer under screens? - react-navigation
- Authentication question - Ruby google-cloud-ruby
- Allow native SDK options to be specified by user without needing to re-initialize native SDK. - TypeScript sentry-react-native
- [question] 502/499 errors when calling Centrifugo API - centrifugo
- Can you update the readme to include Windows users with private repo dependencies? - docker