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

add support to select chart version in the helm install form #5718

Merged

Conversation

debsmita1
Copy link
Contributor

@debsmita1 debsmita1 commented Jun 10, 2020

JIRA story:
https://issues.redhat.com/browse/ODC-4086

Solution Description:

  • added chart version dropdown in the helm install form
  • added a property named isYAMLDirty in HelmInstallUpgradeFormData to check if changes have been to the YAML editor
  • when chart version is changed after making changes in the YAML editor, an alert modal pops up
  • the Chart Version dropdown title now shows the chart version concatenated with the App version
  • the Chart Version dropdown is disabled if there are no chart versions or only one chart version is present

GIF:
helm-install

@openshift-ci-robot openshift-ci-robot added component/dev-console Related to dev-console component/shared Related to console-shared labels Jun 10, 2020
}) => {
const { chartName, chartVersion } = values;
const handleYAMLChange = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is getting called on every keystroke of user in the yaml editor which is not required. Better to have a check using initialValues.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's also an issue with this approach where user changes something in YAML but reverts it back, it will still consider the YAML dirty while technically it's not dirty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed the logic

<YAMLEditorField
name="chartValuesYAML"
onSave={handleSubmit}
onChange={() => handleYAMLChange()}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
onChange={() => handleYAMLChange()}
onChange={handleYAMLChange}

@@ -179,6 +176,7 @@ const HelmInstallUpgradePage: React.FunctionComponent<HelmInstallUpgradePageProp
{(props) => (
<HelmInstallUpgradeForm
{...props}
appVersion={appVersion}
chartHasValues={chartHasValues}
submitLabel={helmAction}
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to just send helmAction instead. Using submitLabel in the other components like helmAction seems weird.

onAccept();
return Promise.resolve();
},
cancel: () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need to set any value on cancel?

Copy link
Contributor Author

@debsmita1 debsmita1 Jun 11, 2020

Choose a reason for hiding this comment

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

because, when I select an option from the dropdown the chartVersion is immediately set to the selected option and then the modal opens up. Now, if I cancel the modal the chart version is not reset to the initial version, which is why I had to manually reset it by setting the chartversion value to the initial version.

@abhinandan13jan
Copy link
Contributor

update tests pls, there is a failing suite

@debsmita1 debsmita1 force-pushed the helm-install-chart-version branch 4 times, most recently from 351f6a6 to c214674 Compare June 11, 2020 14:16
Copy link
Contributor

@rohitkrai03 rohitkrai03 left a comment

Choose a reason for hiding this comment

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

EDIT - Sorry, wrong PR. Need to test this one locally first.

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 11, 2020
@rohitkrai03
Copy link
Contributor

/lgtm cancel
/approve cancel

Added lgtm to wrong PR. Need to verify this locally.

@openshift-ci-robot openshift-ci-robot removed lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 11, 2020
submitLabel={submitLabel}
disableSubmit={(chartVersion && !dirty) || status?.isSubmitting || !_.isEmpty(errors)}
submitLabel={helmAction}
disableSubmit={isSubmitDisabled}
Copy link
Contributor

Choose a reason for hiding this comment

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

The submit button is not getting disabled now while submitting the form.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor

@rohitkrai03 rohitkrai03 left a comment

Choose a reason for hiding this comment

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

/lgtm

Verified locally. Works fine.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 15, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: debsmita1, rohitkrai03

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 15, 2020
@openshift-merge-robot openshift-merge-robot merged commit 72622ef into openshift:master Jun 15, 2020
@spadgett spadgett added this to the v4.6 milestone Jun 15, 2020
@debsmita1
Copy link
Contributor Author

/kind feature

@openshift-ci-robot openshift-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jun 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/dev-console Related to dev-console component/shared Related to console-shared kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants