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

[ui] Set AutoBuild and DeployByDefault #7051

Merged
merged 12 commits into from
Sep 1, 2023
Merged

[ui] Set AutoBuild and DeployByDefault #7051

merged 12 commits into from
Sep 1, 2023

Conversation

feloy
Copy link
Contributor

@feloy feloy commented Aug 29, 2023

What type of PR is this:

/kind feature

What does this PR do / why we need it:

Which issue(s) this PR fixes:

Fixes #7021

PR acceptance criteria:

  • Unit test

  • Integration test

  • Documentation

How to test changes / Special notes to the reviewer:

@openshift-ci openshift-ci bot added the kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation label Aug 29, 2023
@netlify
Copy link

netlify bot commented Aug 29, 2023 •
edited
Loading

✅ Deploy Preview for odo-docusaurus-preview canceled.

Name Link
🔨 Latest commit b05c7c4
🔍 Latest deploy log https://app.netlify.com/sites/odo-docusaurus-preview/deploys/64f08c3eefcf3200080db8cf

@openshift-ci openshift-ci bot requested review from kadel and rm3l August 29, 2023 13:01
@feloy feloy removed the request for review from kadel August 29, 2023 13:07
@odo-robot
Copy link

odo-robot bot commented Aug 29, 2023 •
edited
Loading

OpenShift Unauthenticated Tests on commit 6623b2e finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Aug 29, 2023 •
edited
Loading

NoCluster Tests on commit 6623b2e finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Aug 29, 2023 •
edited
Loading

Unit Tests on commit 6623b2e finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Aug 29, 2023 •
edited
Loading

Validate Tests on commit 6623b2e finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Aug 29, 2023 •
edited
Loading

Kubernetes Tests on commit 6623b2e finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Aug 29, 2023 •
edited
Loading

Windows Tests (OCP) on commit 6623b2e finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Aug 29, 2023 •
edited
Loading

OpenShift Tests on commit 6623b2e finished successfully.
View logs: TXT HTML

@feloy feloy closed this Aug 29, 2023
@feloy feloy reopened this Aug 29, 2023
@odo-robot
Copy link

odo-robot bot commented Aug 29, 2023 •
edited
Loading

Kubernetes Docs Tests on commit 7ff38b7 finished successfully.
View logs: TXT HTML

rm3l
rm3l reviewed Aug 30, 2023
View reviewed changes
Copy link
Member

@rm3l rm3l left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall, but I was wondering about the case of components loaded from the Devfile, without autoBuild/deployByDefault set and not referenced.
Currently, odo automatically applies them at startup (per devfile/api#852 (comment)), but the UI shows them as not built/deployed at startup, which might sound confusing, I guess.

image

image

So shouldn't we at least show them in the UI as built/deployed at startup?

@feloy feloy requested a review from rm3l August 30, 2023 11:18
@feloy feloy force-pushed the feature-7021/ui-autobuild-deploybydefault branch from 0cd1c1c to b78ff6d Compare August 30, 2023 11:42
rm3l
rm3l requested changes Aug 30, 2023
View reviewed changes
Copy link
Member

@rm3l rm3l left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall, but I was wondering about the case of components loaded from the Devfile, without autoBuild/deployByDefault set and not referenced. Currently, odo automatically applies them at startup (per devfile/api#852 (comment)), but the UI shows them as not built/deployed at startup, which might sound confusing, I guess.

So shouldn't we at least show them in the UI as built/deployed at startup?

Looks good on the UI with the latest changes, but I just noticed one slight side effect.

The YAML view currently loads the Devfile with all boolean fields set to their default values if they were not set in the source Devfile, which causes an issue here.
For example, if the Devfile contains an orphan Image component without autoBuild set (so built on startup), the YAML view will have the field set to false. And when persisting it back to the filesystem, this Image component will no longer be applied automatically (because autoBuild is false).

In the source Devfile:

- image:
   imageName: my-image
   dockerfile:
     uri: Containerfile
  name: my-image-cmp

In the UI:
image

I think we should tell the Devfile parser not to automatically set default boolean values when they are unset. See devfile/library#169
And I guess this would also fix some other boolean fields like dedicatedPod, secure, ... that get automatically added when saving the Devfile from the UI to the disk.

@feloy
Copy link
Contributor Author

feloy commented Aug 31, 2023

LGTM overall, but I was wondering about the case of components loaded from the Devfile, without autoBuild/deployByDefault set and not referenced. Currently, odo automatically applies them at startup (per devfile/api#852 (comment)), but the UI shows them as not built/deployed at startup, which might sound confusing, I guess.
So shouldn't we at least show them in the UI as built/deployed at startup?

Looks good on the UI with the latest changes, but I just noticed one slight side effect.

The YAML view currently loads the Devfile with all boolean fields set to their default values if they were not set in the source Devfile, which causes an issue here. For example, if the Devfile contains an orphan Image component without autoBuild set (so built on startup), the YAML view will have the field set to false. And when persisting it back to the filesystem, this Image component will no longer be applied automatically (because autoBuild is false).

The definition of this autoBuild field is not clear to me. Is it a three-state value?

  • true: always build at startup
  • false: never build at startup (even if orphan)
  • undefined: build if orphan only

Or two-states?

  • true: always build at startup
  • false: build if orphan only
  • undefined: defaults to false (build if orphan only)

@rm3l @kadel @elsony wdyt?

@rm3l
Copy link
Member

rm3l commented Aug 31, 2023

LGTM overall, but I was wondering about the case of components loaded from the Devfile, without autoBuild/deployByDefault set and not referenced. Currently, odo automatically applies them at startup (per devfile/api#852 (comment)), but the UI shows them as not built/deployed at startup, which might sound confusing, I guess.
So shouldn't we at least show them in the UI as built/deployed at startup?

Looks good on the UI with the latest changes, but I just noticed one slight side effect.
The YAML view currently loads the Devfile with all boolean fields set to their default values if they were not set in the source Devfile, which causes an issue here. For example, if the Devfile contains an orphan Image component without autoBuild set (so built on startup), the YAML view will have the field set to false. And when persisting it back to the filesystem, this Image component will no longer be applied automatically (because autoBuild is false).

The definition of this autoBuild field is not clear to me. Is it a three-state value?

  • true: always build at startup
  • false: never build at startup (even if orphan)
  • undefined: build if orphan only

Yes, to me, it is a 3-state value and is currently implemented as above (per devfile/api#852 (comment)). So if it is explicitly set to false, it will never be applied at startup even if orphan.

@feloy
Copy link
Contributor Author

feloy commented Aug 31, 2023

LGTM overall, but I was wondering about the case of components loaded from the Devfile, without autoBuild/deployByDefault set and not referenced. Currently, odo automatically applies them at startup (per devfile/api#852 (comment)), but the UI shows them as not built/deployed at startup, which might sound confusing, I guess.
So shouldn't we at least show them in the UI as built/deployed at startup?

Looks good on the UI with the latest changes, but I just noticed one slight side effect.
The YAML view currently loads the Devfile with all boolean fields set to their default values if they were not set in the source Devfile, which causes an issue here. For example, if the Devfile contains an orphan Image component without autoBuild set (so built on startup), the YAML view will have the field set to false. And when persisting it back to the filesystem, this Image component will no longer be applied automatically (because autoBuild is false).

The definition of this autoBuild field is not clear to me. Is it a three-state value?

  • true: always build at startup
  • false: never build at startup (even if orphan)
  • undefined: build if orphan only

Yes, to me, it is a 3-state value and is currently implemented as above (per devfile/api#852 (comment)). So if it is explicitly set to false, it will never be applied at startup even if orphan.

OK, so I will introduce a 3-states button for the user to be able to select one of them

@feloy feloy force-pushed the feature-7021/ui-autobuild-deploybydefault branch from 1e98b99 to 3606681 Compare August 31, 2023 09:17
@sonarcloud
Copy link

sonarcloud bot commented Aug 31, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@feloy feloy requested a review from rm3l August 31, 2023 14:53
rm3l
rm3l approved these changes Sep 1, 2023
View reviewed changes
Copy link
Member

@rm3l rm3l left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works great with the latest changes - thanks!

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Sep 1, 2023
@rm3l
Copy link
Member

rm3l commented Sep 1, 2023

/retest

@openshift-merge-robot openshift-merge-robot merged commit e9dbded into redhat-developer:main Sep 1, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rm3l rm3l rm3l approved these changes

Assignees

@rm3l rm3l

Labels
kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation lgtm Indicates that a PR is ready to be merged. Required by Prow.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[UI] AutoBuild / deployByDefault support
3 participants
@feloy @rm3l @openshift-merge-robot