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

CONSOLE-922: Support AppliedClusterResourceQuota for normal users #10137

Merged
merged 1 commit into from Dec 1, 2021

Conversation

rebeccaalpert
Copy link
Contributor

@rebeccaalpert rebeccaalpert commented Sep 24, 2021

List Pages

I added logic so the resource link knows what the current namespace is. We need to pass the user's current namespace to the resource link if it's an ACRQ because the ACRQ itself doesn't supply namespace metadata.

Scoped namespace:
Screen Shot 2021-09-24 at 5 33 14 PM

All namespaces:
Screen Shot 2021-09-24 at 5 34 45 PM

Support for search page (links work):
Screen Shot 2021-09-28 at 1 14 30 PM

Support for API explorer page (instances links work):
Screen Shot 2021-09-28 at 1 15 14 PM

Details Page

New details page:
Screen Shot 2021-09-28 at 4 13 56 PM

New total used field (total used is cluster scope and used is scoped to the current namespace):
Screen Shot 2021-09-28 at 1 07 06 PM

We show ACRQs in the Resource Quota card in the project overview:
Screen Shot 2021-10-04 at 2 13 08 PM
Screen Shot 2021-10-04 at 2 13 15 PM

I'm basing the meat of the list page part of the PR on some work Sam did a few years ago: https://github.com/spadgett/console/commits/applied-cluster-quota.

Fixes https://issues.redhat.com/browse/CONSOLE-922.

How to test:

  1. Add new user (we are calling this user rebecca here) and login
  2. Create a new project: oc new-project ralpert-test
  3. Log in as admin
  4. Create a new role for them: oc create role rqviewer --verb=get,list,watch --resource=resourcequota,appliedclusterresourcequota -n ralpert-test
  5. Assign the role: oc adm policy add-role-to-user rqviewer rebecca --role-namespace=ralpert-test
  6. Create a cluster resource quota:
oc create clusterquota for-user \
     --project-annotation-selector openshift.io/requester=rebecca \
     --hard pods=10 \
     --hard secrets=20
  1. Change user to rebecca and run oc get appliedclusterresourcequota. If you can see an ACRQ, you should see it in the UI list.
  2. The link should take you to a custom ACRQ details page.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 24, 2021
@openshift-ci openshift-ci bot added the component/shared Related to console-shared label Sep 24, 2021
@openshift-ci openshift-ci bot added the kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated label Sep 24, 2021
@rebeccaalpert rebeccaalpert force-pushed the applied-crq branch 3 times, most recently from 740be0e to 83e5c01 Compare September 29, 2021 15:50
@rebeccaalpert rebeccaalpert changed the title [WIP] CONSOLE-922: Support AppliedClusterResourceQuota for normal users CONSOLE-922: Support AppliedClusterResourceQuota for normal users Sep 29, 2021
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 29, 2021
frontend/packages/console-shared/src/constants/common.ts Outdated Show resolved Hide resolved
frontend/public/components/default-resource.tsx Outdated Show resolved Hide resolved
frontend/public/components/factory/table.tsx Outdated Show resolved Hide resolved
frontend/public/components/resource-quota.jsx Outdated Show resolved Hide resolved
frontend/public/components/resource-quota.jsx Outdated Show resolved Hide resolved
frontend/public/components/resource-quota.jsx Outdated Show resolved Hide resolved
@rebeccaalpert
Copy link
Contributor Author

With suggested "always show ACRQ behavior":

Admin:
Screen Shot 2021-09-30 at 1 13 58 PM

Regular user:
Screen Shot 2021-09-30 at 1 15 33 PM

I addressed PR feedback where possible. @spadgett - I moved the ACRQ logic in the table row out into its own table row; let me know if you meant something else in your comment. Please also let me know if you have thoughts on the table component if you're against passing the namespace down.

I also added a test for the ACRQ ResourceUsageRow.

@spadgett
Copy link
Member

I addressed PR feedback where possible. @spadgett - I moved the ACRQ logic in the table row out into its own table row; let me know if you meant something else in your comment. Please also let me know if you have thoughts on the table component if you're against passing the namespace down.

We definitely don't want to pass namespace down as a prop. Don't tables have a customData prop you can use?

An alternative that might be better is transitioning to the new table API: #8829

@spadgett
Copy link
Member

@rebeccaalpert We should update the project overview page as well to show cluster quotas.

@rebeccaalpert rebeccaalpert force-pushed the applied-crq branch 2 times, most recently from 7ed3442 to c2da38d Compare October 1, 2021 19:35
@openshift-ci openshift-ci bot added the component/dashboard Related to dashboard label Oct 1, 2021
@rebeccaalpert
Copy link
Contributor Author

All set @spadgett. I added ACRQs to the project dashboard.

Screen Shot 2021-10-01 at 3 35 11 PM

@spadgett
Copy link
Member

spadgett commented Oct 1, 2021

@rebeccaalpert Sorry, I wasn't clear. I was thinking of the ResourceQuotaCard on the project dashboard:

https://github.com/openshift/console/blob/22c6951efe7c4bca87f3f934063b9f4dcb0a4058/frontend/public/components/dashboard/project-dashboard/resource-quota-card.tsx

frontend/public/components/default-resource.tsx Outdated Show resolved Hide resolved
frontend/public/components/resource-quota.jsx Outdated Show resolved Hide resolved
frontend/public/components/resource-quota.jsx Outdated Show resolved Hide resolved
@rebeccaalpert rebeccaalpert force-pushed the applied-crq branch 2 times, most recently from 7aac202 to 46dcad2 Compare October 5, 2021 13:28
@rebeccaalpert
Copy link
Contributor Author

rebeccaalpert commented Oct 5, 2021

This is all set @spadgett.

I defined an ACRQ list page and added ACRQs to the ResourceQuotaCard:

Screen Shot 2021-10-04 at 2 13 08 PM

Screen Shot 2021-10-04 at 2 13 15 PM

I updated the behavior for the ResourceQuotas list page. ClusterResourceQuotas are displayed on the ResourceQuotas list page if "all namespaces" is selected. Otherwise, ACRQs are shown. ACRQs now link to the relevant CRQ if users can list CRQs.

Screen Shot 2021-09-28 at 4 13 56 PM

@spadgett
Copy link
Member

@rebeccaalpert We should add some integration tests to cover this. It's an important function that we don't test manually often during development.

@rebeccaalpert
Copy link
Contributor Author

Pushed changes from this morning; still needs integration tests.

Copy link
Member

@TheRealJon TheRealJon left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot 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 Nov 22, 2021
@rebeccaalpert
Copy link
Contributor Author

rebeccaalpert commented Nov 22, 2021

/assign @opayne1
for docs approval

/assign @yapei
for QE approval

/assign @RickJWagner
for px approval

@openshift-bot
Copy link
Contributor

/retest-required

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

@rebeccaalpert rebeccaalpert added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 22, 2021
@rebeccaalpert
Copy link
Contributor Author

Putting on hold because the merge queue is blocked due to a test flake. This is ready for docs, product, and QE review though.

@rebeccaalpert rebeccaalpert removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 23, 2021
@openshift-bot
Copy link
Contributor

/retest-required

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

@RickJWagner
Copy link

/label px-approved

@openshift-ci openshift-ci bot added the px-approved Signifies that Product Support has signed off on this PR label Nov 23, 2021
@openshift-bot
Copy link
Contributor

/retest-required

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

@opayne1
Copy link
Contributor

opayne1 commented Nov 24, 2021

/label docs-approved

@openshift-ci openshift-ci bot added the docs-approved Signifies that Docs has signed off on this PR label Nov 24, 2021
@schituku
Copy link

/lgtm

@schituku
Copy link

/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Nov 30, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 30, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rebeccaalpert, schituku, TheRealJon, zherman0

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

/retest-required

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

@openshift-merge-robot openshift-merge-robot merged commit 9d0fedc into openshift:master Dec 1, 2021
@spadgett spadgett added this to the v4.10 milestone Dec 8, 2021
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/core Related to console core functionality component/dashboard Related to dashboard component/shared Related to console-shared docs-approved Signifies that Docs has signed off on this PR kind/cypress Related to Cypress e2e integration testing 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