Repository navigation
Boolean properties are not being overridden #545
Description
Activity
I just ran a test that successfully overrode the boolean property
MountSourcesunder a container component. The difference between that property and the others mentioned above is that it's a pointer to bool rather than just a bool itself.
This problem surfaces when trying to override a
truevalue with afalsevalue. When afalsevalue is set, it's evaluated as an empty value upon marshalling and subsequently omitted from the patch content since we are specifying theomitemptyoption on these properties. We can fix it the same wayMountSourceswas fixed from v1 to v2 by using boolean pointers but it will change the way clients access these values if they are relying on them.Affected properties are:
Commands
- apply/group/isDefault
- composite/parallel
- exec/hotReloadCapable
Components
- container/dedicatedPod
- [container, kubernetes, openshift]/endpoints/secure
- volume/ephemeral
@kadel @amisevsk, do you know if
odo,che, anddevworkspaceswill be impacted by these proposed changes?DWO would probably be impacted but it's a transparent change to the end-user so I'm fine with doing necessary updates when we bump our dependency.
Reacted by Kim Tsaoodo will be in the same situation as DWO. When odo updates to this it will require a few changes, but it will be all straight forward.
I don't expect any big issues on odo side with this.Reacted by Kim TsaoI pulled the PR back from review after discussions with the team about enforcing default values since this is not done in the api or library packages. This means we will be forcing consumers to perform nil checks to set the default values according to spec. I reached out to @kadel to discuss the impact to ODO and he suggested that we implement a Getter method to return the default values instead. I think this is a good approach, but there is a limitation where we can't make the boolean field private because we also rely on JSON marshalling which only handles exported fields hence, we can't effectively prevent consumers from using these fields directly when the Getter method is available. While this is not ideal, we can write up something in the API description saying the preferred way to access these values is via the Getter methods. I'm not sure if Setter methods would be useful but if we are planning to tell consumers to refrain from using boolean fields directly, then we should probably add these as well.
@amisevsk , @sleshchenko , @davidfestal as owners of this repo and api consumers, any thoughts on this approach?
any thoughts on this approach?
I think a getter method is the wrong approach, as we almost never see this sort of thing in Go. From a k8s API perspective, there are already multiple places where dealing with pointers to base types are used -- it's just the tedium that comes with how serialization is implemented in Go. I can understand however that it's more of a problem for odo, etc., where the k8s api doesn't fill defaults during processing.
Would dropping
omitemptyfrom the json tag be a valid workaround?@amisevsk, Effective Go explains that getters can be used when necessary, but we should be looking to implement them for private fields. I did look into removing
omitemptyon straight booleans but that broke the parent override scenario where an unset, default value offalsecould override a parent value. I believe overrides were intended to operate on explicitly set values so this would not work.The K8s implementation for setting default values is to use
defaulting functionswhich get called during deserialization (ref: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api_changes.md#edit-defaultsgo) This seems to be a pretty big change if we were to adopt something similar so at this point, maybe it's best to separate out the two issues and just fix the override bug that was originally reported. We can track the setting of default values in a separate issue so we can continue to mull over it.I need to make corresponding changes to the library
/kind bug
Which area this bug is related to?
/area api
What versions of software are you using?
N/A
Bug Summary
Describe the bug:
I have a main devfile that contains parent overrides but some properties (
isDefault,hotReloadCapable) under theexeccommand do not get overridden. Similar observation for theparallelproperty under thecompositecommand. I wonder if there's an issue with merging boolean propertiesTo Reproduce:
Main devfile:
Test_Parent1.yaml.zip
Parent devfile:
Parent1.yaml.zip
Expected behavior
properties should be overridden to false
Any logs, error output, screenshots etc? Provide the devfile that sees this bug, if applicable.
Here is the result of the flattenedParent in debug mode:
Additional context
Any workaround?
Suggestion on how to fix the bug