Repository navigation
Devfile Parser should not set default values when flattening is true #1067
Description
Activity
- added a commit that references this issue
on Mar 21, 2023 Just to clarify, my comment about this scenario potentially being a bug applied to the unflattening scenario. The intentions were to always preserve backward compatibility wherever possible in our apis and library so if we do not set default values at all, we will be forcing clients set them: #545 (comment). What you're proposing is a behavioural change and will impact other clients and will require further discussion
Just to clarify, my comment about this scenario potentially being a bug applied to the unflattening scenario. The intentions were to always preserve backward compatibility wherever possible in our apis and library so if we do not set default values at all, we will be forcing clients set them: #545 (comment).
What you're proposing is a behavioural change and will impact other clients and will require further discussionThanks for clarifying, @kim-tsao!
Indeed, if we agree this is a bug applied to the unflattering scenario, then it would make sense to set default values in all cases.
But in this case, maybe clients could be given the option to skip setting default values? So they can decide on their own how to handle unset fields..Just to clarify, my comment about this scenario potentially being a bug applied to the unflattening scenario. The intentions were to always preserve backward compatibility wherever possible in our apis and library so if we do not set default values at all, we will be forcing clients set them: #545 (comment).
What you're proposing is a behavioural change and will impact other clients and will require further discussionThanks for clarifying, @kim-tsao!
Indeed, if we agree this is a bug applied to the unflattering scenario, then it would make sense to set default values in all cases. But in this case, maybe clients could be given the option to skip setting default values? So they can decide on their own how to handle unset fields..
@rm3l , I'll have to do some more investigation to make sure our parent override scenarios do not break. In addition, we discussed this in today's team call and agreed that if we do make a breaking change that it might be best to do so now when we don't have many clients adopting the 2.2 updates. Impacts to pre-2.2 boolean properties were minimal too, since
odowas the only client affected.I'll also look into your suggestion to allow clients to skip setting default values as a way to avoid breaking changes
Thanks, Kim!
@kim-tsao I can confirm the changes in devfile/library#169 work fine for us, by using the new
SetBooleanDefaultsoption. Thanks.Reacted by Kim Tsao- added 2 commits that reference this issue
on Mar 31, 2023 - added a commit that references this issue
on Apr 5, 2023
Which area this feature is related to?
/kind bug
Which area this bug is related to?
/area library
What versions of software are you using?
Go project
Operating System and version:
Fedora 37
Go Pkg Version:
github.com/devfile/library/v2@f87c6926afe80be8abb9fbb83ebfa751f6f93430
Bug Summary
Describe the bug:
While working on adding support for
autoBuildanddeployByDefaultinodo(redhat-developer/odo#5694), I needed to handle the case where those fields are not explicitly set in the Devfile and the components are not referenced by anyapplycommands (see #852 (comment)).But currently, the parser automatically sets unset fields to their default values (
falsehere) when called with the option to flatten the Devfile:https://github.com/devfile/library/blob/bd4a12f272573de1ab254f3e138a9b55904af620/pkg/devfile/parser/parse.go#L147-L153
On the other hand, parsing without flattening makes our tests using child and parent Devfiles fail (mainly because the child Devfile was missing other definitions like commands or components - see the logs here).
To Reproduce:
See this sample project that highlights the issue: https://github.com/rm3l/bug-devfile-parser-fields-not-set
Expected behavior
As discussed during the community call (on March 20, 2023), flattening should not dictate whether default values should be set or not. It should be up to the clients of the library to decide how to handle unset fields. This is especially necessary since fields like
autoBuildanddeployByDefaulthave 3 states (true,false, andnil) with different behaviors that need to be implemented client-side.Any logs, error output, screenshots etc? Provide the devfile that sees this bug, if applicable
https://github.com/rm3l/bug-devfile-parser-fields-not-set/actions
Additional context
—
Any workaround?
—
Suggestion on how to fix the bug
—