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

ODC-7172: Update helm terms from install/uninstall to create/delete #12337

Merged
merged 1 commit into from Dec 14, 2022
Merged

ODC-7172: Update helm terms from install/uninstall to create/delete #12337

merged 1 commit into from Dec 14, 2022

Conversation

PatAKnight
Copy link
Contributor

Fixes: ODC-7172

Description:
This PR updates the Helm terminology to better align with some of the other options available within the developer catalog. The goal is to update the text from "Install" to "Create" and "Uninstall" to "Delete." There is also an addtional text update for the "Install Helm Chart" page that has changed to "Create Helm Release." Also, test cases and e2e scenarios that originally relied on install/uninstall have been updated to now rely on create/delete.

Screenshots:
catalog-side-panel
Create in catalog side panel

create-helm-release-page
Create Helm Release page

delete-helm-release-action
Delete Helm Release action on details page

delete-helm-release-list-page
Delete Helm Release action on list page

delete-helm-release-modal
Delete Helm Release modal

empty-helm-release-list-page
Empty Helm Release list page

Unit Test Coverage:
Screenshot from 2022-12-02 14-16-05

@openshift-ci openshift-ci bot added component/dev-console Related to dev-console component/helm Related to helm-plugin kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated labels Dec 2, 2022
@PatAKnight
Copy link
Contributor Author

PatAKnight commented Dec 2, 2022 •
edited

/cc @debsmita1
/cc @serenamarie125

@openshift-ci openshift-ci bot requested a review from debsmita1 December 2, 2022 19:32
@PatAKnight
Copy link
Contributor Author

/retest

serenamarie125
serenamarie125 approved these changes Dec 5, 2022
View reviewed changes
Copy link
Contributor

@serenamarie125 serenamarie125 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for making these updates

@PatAKnight
Copy link
Contributor Author

/retest

1 similar comment
@PatAKnight
Copy link
Contributor Author

/retest

@debsmita1
Copy link
Contributor

The Release name helptext , should it be A unique name for the Helm release ?

Screenshot 2022-12-06 at 11 21 59 PM

@debsmita1
Copy link
Contributor

/cc @sanketpathak

@PatAKnight
Copy link
Contributor Author

@debsmita1 yes, I agree that it should be A unique name for the Helm release. Have updated to match that.

sanketpathak
sanketpathak reviewed Dec 7, 2022
View reviewed changes
frontend/packages/dev-console/integration-tests/support/pageObjects/add-flow-po.ts
@@ -175,6 +175,7 @@ export const catalogPO = {
installHelmChart: {
logo: 'h1.co-clusterserviceversion-logo__name__clusterserviceversion',
install: '[data-test-id="submit-button"]',
create: '[data-test-id="submit-button"]',
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use install(from line 177) having the same data-test-id or change the name for install to create

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed the install in favor of create.

sanketpathak
sanketpathak reviewed Dec 7, 2022
View reviewed changes
frontend/packages/dev-console/integration-tests/support/constants/pageTitle.ts Outdated
InstallHelmCharts: 'Install Helm Chart',
CreateHelmRelease: 'Create Helm Release',
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need InstallHelmCharts as we are shifting to CreateHelmRelease?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No we do not, have went in and removed it.

@PatAKnight
Copy link
Contributor Author

/retest

@debsmita1
Copy link
Contributor

/lgtm

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 8, 2022
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 8, 2022
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 8, 2022
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 8, 2022
@debsmita1
Copy link
Contributor

/lgtm

@debsmita1
Copy link
Contributor

/label docs-approved
/label px-approved

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 12, 2022
@openshift-ci openshift-ci bot added docs-approved Signifies that Docs has signed off on this PR px-approved Signifies that Product Support has signed off on this PR labels Dec 12, 2022
@debsmita1
Copy link
Contributor

/assign @jerolimov

@sanketpathak
Copy link
Contributor

@PatAKnight Why are we not changing the install on the empty helm page
Screenshot from 2022-12-12 16-24-54

@PatAKnight
Copy link
Contributor Author

@sanketpathak This was mainly to get the helm charts to match with the rest of the developer catalog options. Also, @serenamarie125 wanted the empty helm page text to match the Helm Chart card on the +Add page.

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 12, 2022
@sanketpathak
Copy link
Contributor

sanketpathak commented Dec 12, 2022 •
edited

Thanks @PatAKnight
Verified on cluster created on pr
Screenshot from 2022-12-12 16-32-53
Screenshot from 2022-12-12 16-35-01
Screenshot from 2022-12-12 16-24-54
Screenshot from 2022-12-12 16-21-19
/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Dec 12, 2022
@PatAKnight
Copy link
Contributor Author

/retest

@debsmita1
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 13, 2022
jerolimov
jerolimov approved these changes Dec 13, 2022
View reviewed changes
Copy link
Member

@jerolimov jerolimov left a comment

Choose a reason for hiding this comment

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

/retest
/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 13, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: debsmita1, jerolimov, PatAKnight, serenamarie125

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 openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 13, 2022
@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 6fc6b6c and 2 for PR HEAD 3e4e3a8 in total

@PatAKnight
Copy link
Contributor Author

/retest

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 13, 2022

@PatAKnight: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-merge-robot openshift-merge-robot merged commit 25467aa into openshift:master Dec 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sanketpathak sanketpathak sanketpathak left review comments

@jerolimov jerolimov jerolimov approved these changes

@serenamarie125 serenamarie125 serenamarie125 approved these changes

@christianvogt christianvogt Awaiting requested review from christianvogt

@hemantsaini-7 hemantsaini-7 Awaiting requested review from hemantsaini-7

@debsmita1 debsmita1 Awaiting requested review from debsmita1

Assignees

@jerolimov jerolimov

@debsmita1 debsmita1

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/dev-console Related to dev-console component/helm Related to helm-plugin docs-approved Signifies that Docs has signed off on this PR kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@PatAKnight @debsmita1 @sanketpathak @openshift-ci-robot @jerolimov @serenamarie125 @openshift-merge-robot