Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set default values for boolean properties #615

Closed
kim-tsao opened this issue Sep 9, 2021 · 1 comment
Closed

Set default values for boolean properties #615

kim-tsao opened this issue Sep 9, 2021 · 1 comment
Assignees
Labels
area/api Enhancement or issue related to the api/devfile specification breaking identifies changes that would break clients

Comments

@kim-tsao
Copy link
Contributor

kim-tsao commented Sep 9, 2021

Which area this feature is related to?

/area api

Which functionality do you think we should add?

Why is this needed? Is your feature request related to a problem?

The spec mentions default values for a number of boolean properties, with the majority of them being false if unset (exception is MountSources which is true by default). Neither the API or Library packages enforce the setting of default values since this was handled natively by the JSON marshalling package. In order to fix the parent override bug, we will need to switch to using boolean pointers which will cause the default value to be nil. The unfortunate outcome is, our clients will have to bear the responsibility of enforcing the correct default values.

Detailed description:

Describe the solution you'd like

Clients should be able to access boolean properties and get the default values without having to perform nil checks and setting the defaults themselves.

Describe alternatives you've considered

Discussion in parent override bug describes using getters and setters but it's imperfect since clients can still access the fields which need to remain public.

Other areas to investigate:

  • writing our own custom deserializer and setting the defaults there. We need to make sure parent override fields are exempt since we don't want implicitly set values to override
  • Look at setting defaults in the library parser. This may be an easier approach but will only benefit clients who depend on the library.

Additional context

@kim-tsao kim-tsao self-assigned this Sep 9, 2021
@kim-tsao kim-tsao added the area/api Enhancement or issue related to the api/devfile specification label Sep 9, 2021
@amisevsk
Copy link
Contributor

amisevsk commented Sep 14, 2021

A simple solution could be a set of helper functions, e.g.

func GetMountSourcesOrDefault(container *ContainerComponent) bool {
	var mountSources bool
	if devfileContainer.MountSources == nil {
		mountSources = true
	} else {
		mountSources = *devfileContainer.MountSources
	}
	return mountSources
}

This is how we've handled mountSources in DWO. This would also allow more complex rules, e.g. there's a note on the implementation in DWO that mountSources is default true unless dedicatedPod is set (this is unimplemented though).

A custom deserializer could be tricky -- one of Go's big drawbacks is that it's hard to differentiate between nil and the default.

@kim-tsao kim-tsao closed this as completed Nov 4, 2021
@kim-tsao kim-tsao added the breaking identifies changes that would break clients label Sep 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Enhancement or issue related to the api/devfile specification breaking identifies changes that would break clients
Projects
None yet
Development

No branches or pull requests

2 participants