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-7721: Disable developer perspective by default #954

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Lucifergene
Copy link

@Lucifergene Lucifergene commented Dec 18, 2024

Jira: https://issues.redhat.com/browse/ODC-7721

This pull request introduces changes to manage the visibility of different perspectives in the console server configuration. The changes include adding a new type for perspective IDs, updating the configuration builder to handle perspective visibility, and modifying the operator configuration manifest.

Changes to perspective management:

Configuration updates:

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Dec 18, 2024
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Dec 18, 2024

@Lucifergene: This pull request references ODC-7721 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set.

In response to this:

Jira: https://issues.redhat.com/browse/ODC-7721

This pull request includes a small change to the manifests/01-operator-config.yaml file. The change disables the visibility of the 'dev' perspective in the configuration.

Configuration changes:

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 openshift-eng/jira-lifecycle-plugin repository.

Copy link
Contributor

openshift-ci bot commented Dec 18, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Lucifergene
Once this PR has been reviewed and has the lgtm label, please assign therealjon for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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

@spadgett
Copy link
Member

/cc @jhadvig @TheRealJon

@openshift-ci openshift-ci bot requested a review from jhadvig December 18, 2024 14:26
@spadgett
Copy link
Member

I suspect we're need to make a lot of Cypress test updates before we can merge this since many tests switch to the dev perspective.

@vikram-raj
Copy link
Member

I suspect we're need to make a lot of Cypress test updates before we can merge this since many tests switch to the dev perspective.

Yes, we need to update ODC cypress tests otherwise it will break the CI. Here is the story for it ODC-7718 we will get this in first.

@Lucifergene Lucifergene force-pushed the disable-dev-perspective-by-default branch from 5388256 to cfe12f9 Compare December 23, 2024 10:17
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Dec 23, 2024

@Lucifergene: This pull request references ODC-7721 which is a valid jira issue.

In response to this:

Jira: https://issues.redhat.com/browse/ODC-7721

This pull request introduces changes to manage the visibility of different perspectives in the console server configuration. The changes include adding a new type for perspective IDs, updating the configuration builder to handle perspective visibility, and modifying the operator configuration manifest.

Changes to perspective management:

Configuration updates:

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 openshift-eng/jira-lifecycle-plugin repository.

@jhadvig
Copy link
Member

jhadvig commented Jan 2, 2025

Thanks @Lucifergene
It looks like you need to also update the e2e tests. PTAL

Signed-off-by: Lucifergene <[email protected]>
@Lucifergene
Copy link
Author

/label tide/merge-method-squash
/label docs-approved
/label px-approved

@openshift-ci openshift-ci bot added tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. docs-approved Signifies that Docs has signed off on this PR px-approved Signifies that Product Support has signed off on this PR labels Jan 8, 2025
@vikram-raj
Copy link
Member

/retest

@Lucifergene
Copy link
Author

/test e2e-aws-console

@jhadvig jhadvig self-assigned this Jan 10, 2025
@Lucifergene
Copy link
Author

/test e2e-aws-console

@jhadvig
Copy link
Member

jhadvig commented Jan 13, 2025

/retest

1 similar comment
@Lucifergene
Copy link
Author

/retest

Copy link
Contributor

openshift-ci bot commented Jan 15, 2025

@Lucifergene: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-aws-console f34a319 link true /test e2e-aws-console

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-sigs/prow repository. I understand the commands that are listed here.

@Lucifergene
Copy link
Author

Note: No need to run retests on the failing ones.
The ongoing ci/prow/e2e-aws-console failure is caused due to the merging of perspective.

Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

@Lucifergene Any thoughts on how to approach updating the e2e tests in the console repo? Maybe a good first step is to update the tests to use the admin perspective for any nav items that are now exposed there.

@@ -500,6 +500,16 @@ func (b *ConsoleServerCLIConfigBuilder) customization() Customization {
}
}

conf.Perspectives = perspectives
Copy link
Member

Choose a reason for hiding this comment

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

Let's add a unit test that confirms that the dev perspective is not disabled when there is any perspective config.

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this is a little odd since if I add a config to customize perspective foo it could also enable the dev perspective when it was disabled before because there was no config. We might want to change the logic here to only enable the dev perspective if the admin perspective is disabled (or disabled for at least some users due to an access review check).

What do you guys think?

Copy link
Member

Choose a reason for hiding this comment

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

Agree, we should only enable the dev perspective.

@vikram-raj
Copy link
Member

Hi @spadgett, regarding the e2e tests, @sanketpathak is working on updating the e2e tests. Here is the Jira ticket ODC-7718 and PR openshift/console#14684

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-approved Signifies that Docs has signed off on this PR jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. px-approved Signifies that Product Support has signed off on this PR tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants