Boolean properties are not being overridden

[kim-tsao]

/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 the exec command do not get overridden. Similar observation for the parallel property under the composite command. I wonder if there's an issue with merging boolean properties

To Reproduce:

1. Main devfile:
   Test_Parent1.yaml.zip

2. 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

[kim-tsao]

I just ran a test that successfully overrode the boolean property MountSources under 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.

api/pkg/apis/workspaces/v1alpha2/component_container.go

Line 58 in cde1b99

MountSources *bool `json:"mountSources,omitempty"`

[kim-tsao]

This problem surfaces when trying to override a true value with a false value. When a false value is set, it's evaluated as an empty value upon marshalling and subsequently omitted from the patch content since we are specifying the omitempty option on these properties. We can fix it the same way MountSources was 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, and devworkspaces will be impacted by these proposed changes?

[amisevsk]

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.

[kadel]

odo 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.

[kim-tsao]

I 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?

[amisevsk]

@kim-tsao

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 omitempty from the json tag be a valid workaround?

[kim-tsao]

@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 omitempty on straight booleans but that broke the parent override scenario where an unset, default value of false could override a parent value. I believe overrides were intended to operate on explicitly set values so this would not work.

[kim-tsao]

The K8s implementation for setting default values is to use defaulting functions which 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.