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.

Sign up for GitHub

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

Jump to bottom

Devfile Parser should not set default values when flattening is true #1067

Closed
rm3l opened this issue Mar 21, 2023 · 8 comments
Closed

Devfile Parser should not set default values when flattening is true #1067

rm3l opened this issue Mar 21, 2023 · 8 comments
Assignees
@kim-tsao
Labels
area/library Common devfile library for interacting with devfiles kind/bug Something isn't working severity/blocker Issues that prevent developers from working

Comments

@rm3l
Copy link
Member

rm3l commented Mar 21, 2023 •
edited
Loading

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 autoBuild and deployByDefault in odo (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 any apply commands (see #852 (comment)).
But currently, the parser automatically sets unset fields to their default values (false here) 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 autoBuild and deployByDefault have 3 states (true, false, and nil) 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

—
@openshift-ci openshift-ci bot added kind/bug Something isn't working area/library Common devfile library for interacting with devfiles labels Mar 21, 2023
rm3l added a commit to rm3l/devfile-library that referenced this issue Mar 21, 2023
@rm3l
Do not set default values when parsing a Devfile while enabling flatt…
1750982 
…ening

See [1] for more context

[1] devfile/api#1067

Signed-off-by: Armel Soro <asoro@redhat.com>
@rm3l rm3l mentioned this issue Mar 21, 2023
Closed
@kim-tsao
Copy link
Contributor

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

@kim-tsao kim-tsao self-assigned this Mar 21, 2023
@rm3l
Copy link
Member Author

rm3l commented Mar 21, 2023 •
edited
Loading

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

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

@kim-tsao
Copy link
Contributor

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

Thanks 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 odo was the only client affected.

@kim-tsao
Copy link
Contributor

I'll also look into your suggestion to allow clients to skip setting default values as a way to avoid breaking changes

@rm3l
Copy link
Member Author

rm3l commented Mar 22, 2023 •
edited
Loading

Thanks, Kim!

@johnmcollier johnmcollier added the severity/blocker Issues that prevent developers from working label Mar 24, 2023
@kim-tsao kim-tsao mentioned this issue Mar 27, 2023
Merged
5 tasks
@kim-tsao
Copy link
Contributor

@rm3l, I have a PR ready. Please verify if this is the behaviour you're looking for, thanks!

@rm3l
Copy link
Member Author

rm3l commented Mar 28, 2023

@rm3l, I have a PR ready. Please verify if this is the behaviour you're looking for, thanks!

Thanks, @kim-tsao! I'll take a look soon and let you know.

@rm3l
Copy link
Member Author

rm3l commented Mar 30, 2023

@kim-tsao I can confirm the changes in devfile/library#169 work fine for us, by using the new SetBooleanDefaults option. Thanks.

rm3l added a commit to rm3l/odo that referenced this issue Mar 31, 2023
@rm3l
Bump Devfile library to the latest commit at this time (c1b23d2)
4e2d015 
This includes the fix for [1], which provides a way not to set default values automatically

[1] devfile/api#1067
rm3l added a commit to rm3l/odo that referenced this issue Apr 4, 2023
@rm3l
Bump Devfile library to the latest commit at this time (c1b23d2)
035e826 
This includes the fix for [1], which provides a way not to set default values automatically

[1] devfile/api#1067
openshift-merge-robot pushed a commit to redhat-developer/odo that referenced this issue Apr 5, 2023
@rm3l @feloy @valaparthvi
Support autoBuild and deployByDefault on Image and Kubernetes/Ope…
cc2e8de 
…nShift components (#6654)

* Add sample Devfile with multiple autoBuild/deployByDefault combinations

It will be used for integration tests.

* Add helper function to update a given Devfile Command Group

This will allow supporting cases where we need to run a custom command,
but this is not implemented yet on odo (cases with 'odo dev --debug'
and 'odo deploy').
In this case, this helper will allow updating the Devfile for example to
unmark the current default command as non-default, and mark the custom
one as default.

* Add test cases for 'odo dev'

* Add test cases for 'odo deploy'

* Add test cases for 'odo build-images'

'odo build-images' should build all images regardless of the 'autoBuild' property.

* Display the spinner when creating or updating Kubernetes resources

This helps understand what resources are being patched.

* Handle deployByDefault on K8s and OpenShift components

* Add 'devfile.GetImageComponentsToPush' functions that returns the list of image components to auto-create

See devfile/api#852 (comment) for more context.

* Handle automatic image component creation for 'odo dev' on Kubernetes

* Handle automatic image component creation for 'odo dev' on Podman

* Handle automatic image and K8s/OpenShift component creation for 'odo deploy'

* Bump Devfile library to the latest commit at this time (c1b23d2)

This includes the fix for [1], which provides a way not to set default values automatically

[1] devfile/api#1067

* Do not set default values when parsing a Devfile

See [1] for the rationale

[1] https://github.com/redhat-developer/odo/issues/5694\#issuecomment-1465778398

* Add documentation in the Devfile reference page

* Revert "Display the spinner when creating or updating Kubernetes resources"

This reverts commit 6ad073e63cb0e685f165eed767619a90997393a3.

* Avoid re-applying Image components multiple times

'adapter#Push' might get called several times when there are
state transitions (either on the Deployment in the cluster,
or from 'odo').
It might be confusing to apply Image components over and over again
(and also it can be slower if we need to push images to remote registries).

* Move GetK8sAndOcComponentsToPush and GetImageComponentsToPush to libdevfile package

As suggested in review, this should be the responsibility of the devfile library

Co-authored-by: Philippe Martin <phmartin@redhat.com>

* fixup! Handle automatic image and K8s/OpenShift component creation for 'odo deploy'

* fixup! Handle automatic image component creation for 'odo dev' on Podman

* fixup! Avoid re-applying Image components multiple times

* Apply suggestions from code review

Co-authored-by: Parthvi Vala <pvala@redhat.com>

* fixup! Do not set default values when parsing a Devfile

* Fix devfile-deploy-functional-pods.yaml (used in 'odo logs' tests)

Set deployByDefault to false on innerloop-pod component.
Otherwise, since it is not referenced by any apply command,
it will be automatically created by *both* 'odo dev' and 'odo deploy'.

---------

Co-authored-by: Philippe Martin <phmartin@redhat.com>
Co-authored-by: Parthvi Vala <pvala@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kim-tsao kim-tsao

Labels
area/library Common devfile library for interacting with devfiles kind/bug Something isn't working severity/blocker Issues that prevent developers from working
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@rm3l @johnmcollier @kim-tsao