CONSOLE-5061: Add Resource Names column to Role Rules table#15999
CONSOLE-5061: Add Resource Names column to Role Rules table#15999openshift-merge-bot[bot] merged 3 commits intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@krishagarwal278: This pull request references CONSOLE-5061 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.22.0" version, but no target version was set. DetailsIn response to this:
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. |
openshift-ci
bot
added
component/core
|
@krishagarwal278: This pull request references CONSOLE-5061 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.22.0" version, but no target version was set. DetailsIn response to this:
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. |
📝 WalkthroughWalkthroughThis pull request extends RBAC rule table functionality to support the 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/public/components/RBAC/rules.jsx`:
- Around line 131-143: The ResourceNames component currently calls
resourceNames.sort(), which mutates the incoming prop; instead create a copy
before sorting (e.g., use [...resourceNames] or resourceNames.slice()) and call
sort on that copy, then map over the sorted copy to render the rows; update
ResourceNames to avoid mutating the resourceNames prop and preserve
immutability.
🧹 Nitpick comments (1)
frontend/packages/integration-tests-cypress/tests/crud/roles-rolebindings.cy.ts (1)
17-54: Consider adding test data with actualresourceNamesfor more comprehensive coverage.The example roles created in
createExampleRoles()use default YAML that likely doesn't includeresourceNames. To fully validate the new column renders content correctly (not just the header), consider adding a rule withresourceNamespopulated.💡 Example enhancement
yamlEditor.getEditorContent().then((content) => { - newContent = _.defaultsDeep({}, { metadata: { name: roleName } }, safeLoad(content)); + newContent = _.defaultsDeep( + {}, + { + metadata: { name: roleName }, + rules: [ + { + apiGroups: [''], + resources: ['pods'], + resourceNames: ['example-pod'], + verbs: ['get', 'list'], + }, + ], + }, + safeLoad(content), + ); yamlEditor.setEditorContent(safeDump(newContent)).then(() => {Then in the test, you could additionally verify:
cy.contains('td', 'example-pod').should('exist');
| const ResourceNames = ({ resourceNames }) => { | ||
| if (!resourceNames || resourceNames.length === 0) { | ||
| return null; | ||
| } | ||
|
|
||
| const names = resourceNames.sort().map((name) => ( | ||
| <div className="rbac-rule-row" key={name}> | ||
| {name} | ||
| </div> | ||
| )); | ||
|
|
||
| return <div>{names}</div>; | ||
| }; |
There was a problem hiding this comment.
Avoid mutating the resourceNames prop with .sort().
Array.prototype.sort() mutates the original array. While React props should be treated as immutable, this could cause subtle bugs if the same array reference is reused elsewhere. Spread the array first to create a copy.
🔧 Proposed fix
const ResourceNames = ({ resourceNames }) => {
if (!resourceNames || resourceNames.length === 0) {
return null;
}
- const names = resourceNames.sort().map((name) => (
+ const names = [...resourceNames].sort().map((name) => (
<div className="rbac-rule-row" key={name}>
{name}
</div>
));
return <div>{names}</div>;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const ResourceNames = ({ resourceNames }) => { | |
| if (!resourceNames || resourceNames.length === 0) { | |
| return null; | |
| } | |
| const names = resourceNames.sort().map((name) => ( | |
| <div className="rbac-rule-row" key={name}> | |
| {name} | |
| </div> | |
| )); | |
| return <div>{names}</div>; | |
| }; | |
| const ResourceNames = ({ resourceNames }) => { | |
| if (!resourceNames || resourceNames.length === 0) { | |
| return null; | |
| } | |
| const names = [...resourceNames].sort().map((name) => ( | |
| <div className="rbac-rule-row" key={name}> | |
| {name} | |
| </div> | |
| )); | |
| return <div>{names}</div>; | |
| }; |
🤖 Prompt for AI Agents
In `@frontend/public/components/RBAC/rules.jsx` around lines 131 - 143, The
ResourceNames component currently calls resourceNames.sort(), which mutates the
incoming prop; instead create a copy before sorting (e.g., use
[...resourceNames] or resourceNames.slice()) and call sort on that copy, then
map over the sorted copy to render the rows; update ResourceNames to avoid
mutating the resourceNames prop and preserve immutability.
openshift-ci
bot
added
lgtm
|
docs-approval: px-approval: qe-approval: |
|
@krishagarwal278: GitHub didn't allow me to assign the following users: jseseCCS, rh-joshbeverly. Note that only openshift members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. DetailsIn response to this:
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. |
|
/label px-approved |
openshift-ci
bot
added
the
px-approved
|
Tested on cluster launched against the pr, go to anyone role page, check Rules table, there is 'Resource Names' column column, when a rule has 'resourceNames'(could check from role yaml), the value will show up in 'Resource Names' column, otherwise, the 'Resource Names' is empty. /verified by yanpzhan |
openshift-ci-robot
added
the
verified
|
@yanpzhan: This PR has been marked as verified by DetailsIn response to this:
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. |
| "Create Role": "Create Role", | ||
| "API groups": "API groups", | ||
| "Resources": "Resources", | ||
| "Resource Names": "Resource Names", |
There was a problem hiding this comment.
Suggest sentence case for consistency w/other UI labels in this file/across console.
Change: "Resource Names" → "Resource names"
Field labels, table headers, dropdown labels use sentence case (API groups, Delete rule), so this should align w/that pattern.
|
suggested 1 change; otherwise LGTM |
|
/label docs-approved |
openshift-ci
bot
added
the
docs-approved
openshift-ci-robot
removed
the
verified
|
/label tide/merge-method-squash |
openshift-ci
bot
added
the
tide/merge-method-squash
| const names = resourceNames.sort().map((name) => ( | ||
| <div className="rbac-rule-row" key={name}> | ||
| {name} | ||
| </div> | ||
| )); |
There was a problem hiding this comment.
code rabbit is right. sort will mutate the array, but best practice dictates we treat props as read only. we can use toSorted to avoid that
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhadvig, jseseCCS, krishagarwal278, logonoff The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/verified by @yanpzhan @krishagarwal278 |
openshift-ci-robot
added
the
verified
|
@krishagarwal278: This PR has been marked as verified by DetailsIn response to this:
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. |
|
/retest |
1 similar comment
|
/retest |
|
@krishagarwal278: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
7 participants
Reviewers Checklist:
Summary by CodeRabbit
New Features
Tests