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

ODC-6779 - Hide sub-catalog(s) in the developer catalog or the entire dev catalog based on customization #12067

Merged

Conversation

lokanandaprabhu
Copy link
Contributor

@lokanandaprabhu lokanandaprabhu commented Sep 20, 2022

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

Description:
This story is about the customization of developer catalog and sub-catalog based on customization. If all the available sub-catalogs(types) are disabled, complete developer catalog along with the entry points to the catalog will be disabled.

Screenshots:
----When all the sub-catalogs are disabled - Add page----
Screenshot 2022-09-20 at 12 38 17 PM

----When all the sub-catalogs are disabled - Topology page----
Screenshot 2022-09-20 at 12 38 34 PM

-----Quick start when complete developer catalog is disabled-----
Screenshot 2022-09-20 at 12 38 52 PM

------ All services - when all the sub-catalogs(types) are enabled----
Screenshot 2022-09-20 at 2 18 35 PM

------ All services - when Templates and Helm charts sub-catalogs are disabled----
Screenshot 2022-09-20 at 2 18 44 PM

-----Helm Page when helm chart sub-catalog is disabled-----
Screenshot 2022-09-20 at 12 38 01 PM

---- Operator hub description when operator backed service is disabled or complete developer catalog is disabled---
Screenshot 2022-09-20 at 12 39 37 PM

---- If Event source type is disabled, create event source in action menu is hidden----
Screenshot 2022-09-20 at 1 00 52 PM

-----If user try to reach to catalog by entering URL and if that sub-catalog is disabled, then user will see Page not found--
Screenshot 2022-09-20 at 12 46 20 PM

Test cases:

Screenshot 2022-09-20 at 3 26 05 PM

How to test:

If backend API PR's - openshift/api#1256 & openshift/console-operator#676 are not merged then to add the customization use below method,

Pass options as a string to bridge with -developer-catalog-disabled-types or pass a ConsoleConfig yaml with -config option.

Example:

Specify developer catalog types to bridge with ./bin/bridge -developer-catalog-types '{ "state": "Disabled" ,"disabled": ["HelmChart", "Devfile"] }'.

This indicated HelmChart and Devfile sub-catalogs are disabled. Similarly below are the test scenarios,

If the state is Enabled and enabled list is empty, all the types will be shown.
If the state id Enabled and enabled list is non-empty, only the enabled list items will be shown.
If the state is Disabled and disabled list is empty, all the types will be hidden.
If the state id Disabled and disabled list is non-empty, only the disabled list items will be hidden.

To disable the complete developer catalog then in disabled all the available types
should be listed or make the disabled list empty for state: Disabled.

/kind feature

@openshift-ci openshift-ci bot added component/backend Related to backend component/dev-console Related to dev-console component/helm Related to helm-plugin component/knative Related to knative-plugin component/kubevirt Related to kubevirt-plugin component/olm Related to OLM component/sdk Related to console-plugin-sdk component/shared Related to console-shared kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated labels Sep 20, 2022
@lokanandaprabhu
Copy link
Contributor Author

/retest

@lokanandaprabhu
Copy link
Contributor Author

/assign @jerolimov
/assign @invincibleJai
/cc @debsmita1

@lokanandaprabhu
Copy link
Contributor Author

/retest

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 25, 2022
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.

This was just a high level review but we should not update the extensions if it's not needed.

PTAL on my comments below:

Comment on lines 65 to 68
helmRelease: {
label: t('helm-plugin~Helm Release'),
label: isHelmVisible ? t('helm-plugin~Helm Release') : null,
onSelection: () => `/catalog/ns/${namespace}?catalogType=HelmChart`,
},
Copy link
Member

Choose a reason for hiding this comment

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

If the flag is false and we don't want to add an Action, we should not add it completely instead of setting the label to null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the label is null then that action will not be added.

@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 26, 2022
@lokanandaprabhu
Copy link
Contributor Author

Hi @jerolimov / @debsmita1 ,

Updated the PR as per the PR comments.

@invincibleJai
Copy link
Member

/retest

1 similar comment
@lokanandaprabhu
Copy link
Contributor Author

/retest

@lokanandaprabhu lokanandaprabhu force-pushed the feature/ODC6779 branch 3 times, most recently from 3926393 to e5ea31b Compare September 30, 2022 08:54
@lokanandaprabhu
Copy link
Contributor Author

/retest

@lokanandaprabhu
Copy link
Contributor Author

Updated the PR. PTAL. Thanks.

@debsmita1
Copy link
Contributor

The entire dev-catalog is disabled, but I can see Samples option when I click on topology graph
https://user-images.githubusercontent.com/22490998/195789490-c2df4321-a88d-4c1b-82c1-ac5e4a05cccc.mov

@lokanandaprabhu
Copy link
Contributor Author

The entire dev-catalog is disabled, but I can see Samples option when I click on topology graph https://user-images.githubusercontent.com/22490998/195789490-c2df4321-a88d-4c1b-82c1-ac5e4a05cccc.mov

Updated the PR.

Copy link
Member

@vikram-raj vikram-raj left a comment

Choose a reason for hiding this comment

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

Thanks @lokanandaprabhu . This feature works as expected. Just added a couple of nits. PTAL if it makes sense.

frontend/packages/dev-console/src/actions/providers.ts Outdated Show resolved Hide resolved
frontend/packages/dev-console/src/actions/providers.ts Outdated Show resolved Hide resolved
@lokanandaprabhu lokanandaprabhu force-pushed the feature/ODC6779 branch 2 times, most recently from ff83dc0 to 0fa6ab9 Compare October 14, 2022 10:28
@@ -47,7 +48,7 @@ const CatalogServiceProvider: React.FC<CatalogServiceProviderProps> = ({
catalogBadgeProviderExtensions,
extensionsResolved,
] = useCatalogExtensions(catalogId, catalogType);

const [disabledSubCatalogs] = useGetAllDisabledSubCatalogs();
Copy link
Member

Choose a reason for hiding this comment

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

Isn't it easier to filter the items directly in useCatalogExtensions so that it returns already just an array of enabled catalogTypeExtensions?

frontend/packages/dev-console/src/utils/catalog-utils.ts Outdated Show resolved Hide resolved
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.

This PR needs a rebase and I would like that you move the utils as mentioned below.

@jerolimov
Copy link
Member

jerolimov commented Oct 16, 2022

Following our no-FF process propagate the PX and Docs ACK from epic ODC-5897
/label px-approved
/label docs-approved

I add also approve here so that @debsmita1 can do the final review when the files are moved (#12067 (review)) and the PR is rebased.
/approve

@openshift-ci openshift-ci bot added px-approved Signifies that Product Support has signed off on this PR docs-approved Signifies that Docs has signed off on this PR labels Oct 16, 2022
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 16, 2022
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 16, 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 Oct 17, 2022
@lokanandaprabhu
Copy link
Contributor Author

Hi @jerolimov / @debsmita1 ,

I have updated the PR based on PR comments. PTAL. Thanks.

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.

Tested the latest version on a cluster bot instance and it works fine. 👍

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 17, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 17, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jerolimov, lokanandaprabhu

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
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 167ca37 and 2 for PR HEAD 59b26fb in total

@lokanandaprabhu
Copy link
Contributor Author

/retest

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 18, 2022

@lokanandaprabhu: 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 4966fa6 into openshift:master Oct 18, 2022
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/backend Related to backend component/dev-console Related to dev-console component/helm Related to helm-plugin component/knative Related to knative-plugin component/kubevirt Related to kubevirt-plugin component/olm Related to OLM component/sdk Related to console-plugin-sdk component/shared Related to console-shared 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
Development

Successfully merging this pull request may close these issues.

None yet

8 participants