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

feature flag Helm based on HelmRepository CRs #7035

Merged
merged 1 commit into from Dec 1, 2020
Merged

feature flag Helm based on HelmRepository CRs #7035

merged 1 commit into from Dec 1, 2020

Conversation

debsmita1
Copy link
Contributor

@debsmita1 debsmita1 commented Oct 29, 2020 •
edited

JIRA Story:
https://issues.redhat.com/browse/ODC-5024

Root Analysis:
Need a way to enable/disable helm specific navigation items from the console if there are no helm repositories configured

Solution Description:

  • disables the FLAG_OPENSHIFT_HELM flag when there are no HelmChartRepository configured &
  • hides the Helm nav item
  • hides the Helm catalog item from the Add page
  • the helm URLs shows 404
  • hides the Helm filter type from the Dev catalog

GIF/Sceenshot:
helm_ff

@debsmita1
Copy link
Contributor Author

/kind feature

@openshift-ci-robot openshift-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. component/core Related to console core functionality labels Oct 29, 2020
@openshift-ci-robot openshift-ci-robot added the component/dev-console Related to dev-console label Oct 29, 2020
@debsmita1 debsmita1 changed the title feature flag Helm based on HelmRepository CRs [WIP] feature flag Helm based on HelmRepository CRs Oct 30, 2020
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 30, 2020
@debsmita1 debsmita1 changed the title [WIP] feature flag Helm based on HelmRepository CRs feature flag Helm based on HelmRepository CRs Oct 30, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 30, 2020
@debsmita1
Copy link
Contributor Author

/assign @rohitkrai03

rohitkrai03
rohitkrai03 suggested changes Nov 3, 2020
View reviewed changes
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.

Even after helm flag is disabled catalog has a type filter for Helm Charts. Its probably better if we remove that filter item as well when Helm is disabled. @serenamarie125 thoughts on this?

Screenshot from 2020-11-04 00-59-59

@debsmita1
Copy link
Contributor Author

/cc @rachael-phillips

@debsmita1 debsmita1 force-pushed the add-helm-ff branch 2 times, most recently from b3f116f to f94d7d6 Compare November 6, 2020 18:38
rohitkrai03
rohitkrai03 suggested changes Nov 9, 2020
View reviewed changes
frontend/packages/dev-console/src/components/topology/helm/helmTopologyPlugin.tsx Outdated
@@ -32,6 +33,9 @@ export const helmTopologyPlugin: Plugin<HelmTopologyConsumedExtensions> = [
getDataModel: getHelmTopologyDataModel,
isResourceDepicted: getIsHelmResource,
},
flags: {
required: [FLAG_OPENSHIFT_HELM],
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to remove topology visualization if helm is disabled.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 16, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 19, 2020
@jeff-phillips-18
Copy link
Member

We should also add the flag to the Helm Topology plugin: https://github.com/openshift/console/blob/6062e7e96baff020679764676655a8b54d46eb59/frontend/packages/topology/src/helm/helmTopologyPlugin.tsx

rohitkrai03
rohitkrai03 suggested changes Nov 30, 2020
View reviewed changes
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.

@debsmita1 Tested the PR locally. Everything works as expected except the catalog. You need to feature flag the catalog extensions for Helm as well. Update the file /catalog/catalog-plugin.ts in components folder and add the Helm feature flag in helm extensions.

@rohitkrai03
Copy link
Contributor

We should also add the flag to the Helm Topology plugin: https://github.com/openshift/console/blob/6062e7e96baff020679764676655a8b54d46eb59/frontend/packages/topology/src/helm/helmTopologyPlugin.tsx

@jeff-phillips-18 Based on the chats from this thread it seems we would want to show the topology visualization for helm charts regardless of helm flag. Maybe @serenamarie125 can confirm it here as well.

rohitkrai03
rohitkrai03 approved these changes Nov 30, 2020
View reviewed changes
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.

/approve
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 30, 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 Nov 30, 2020
@rohitkrai03
Copy link
Contributor

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

3 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@rohitkrai03
Copy link
Contributor

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 1453644 into openshift:master Dec 1, 2020
@spadgett spadgett added this to the v4.7 milestone Dec 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rohitkrai03 rohitkrai03 rohitkrai03 approved these changes

@abhinandan13jan abhinandan13jan Awaiting requested review from abhinandan13jan

@jeff-phillips-18 jeff-phillips-18 Awaiting requested review from jeff-phillips-18

@rachael-phillips rachael-phillips Awaiting requested review from rachael-phillips

Assignees

@rohitkrai03 rohitkrai03

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/core Related to console core functionality component/dev-console Related to dev-console 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
Milestone
v4.7
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@debsmita1 @jeff-phillips-18 @rohitkrai03 @openshift-ci-robot @openshift-bot @spadgett @openshift-merge-robot