Trim whitespace when splitting security-group annotations - #6535
Conversation
|
Hi @sungupta-llm. Thanks for your PR. I'm waiting for a stolostron member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
📝 WalkthroughWalkthroughGovernance overview annotation parsing now trims whitespace around comma-separated security group and policy names. Regression tests verify standards deduplication and sidebar matching for trimmed annotation values. ChangesGovernance annotation parsing
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
frontend/src/routes/Governance/overview/Overview.test.tsx (3)
19-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a type-only import for
Policy.
Policyis used only as a type in this test; split it intoimport type { Policy }to avoid a runtime import. As per coding guidelines, “Useimport typefor type-only imports to avoid generating runtime imports.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/routes/Governance/overview/Overview.test.tsx` at line 19, Update the import of Policy in Overview.test.tsx to use a type-only import, while keeping PolicyApiVersion and PolicyKind in the regular value import from resources.Source: Coding guidelines
197-210: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the required accessibility assertion.
Capture
containerfromrenderand assertexpect(await axe(container)).toHaveNoViolations()for this new component test. As per coding guidelines, “Every component test must include an accessibility assertion withjest-axe.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/routes/Governance/overview/Overview.test.tsx` around lines 197 - 210, Update the new GovernanceOverview test around the render call to capture the returned container, then await axe(container) and assert it has no accessibility violations. Preserve the existing providers, state initialization, and component rendering while adding the required jest-axe assertion.Source: Coding guidelines
200-203: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMock shared atoms through
useSharedAtoms().This new fixture initializes
policiesStateandmanagedClustersStatethrough direct atom imports; mock theuseSharedAtoms()contract instead. As per coding guidelines, “Mock Recoil atoms viauseSharedAtoms()rather than direct atom imports.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/routes/Governance/overview/Overview.test.tsx` around lines 200 - 203, Update the test fixture’s initializeState setup around the policiesState and managedClustersState references to mock the useSharedAtoms() contract instead of importing or setting shared Recoil atoms directly. Ensure the test receives the fixture values through the mocked hook while preserving the existing policy and managed-cluster data.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@frontend/src/routes/Governance/overview/Overview.test.tsx`:
- Line 19: Update the import of Policy in Overview.test.tsx to use a type-only
import, while keeping PolicyApiVersion and PolicyKind in the regular value
import from resources.
- Around line 197-210: Update the new GovernanceOverview test around the render
call to capture the returned container, then await axe(container) and assert it
has no accessibility violations. Preserve the existing providers, state
initialization, and component rendering while adding the required jest-axe
assertion.
- Around line 200-203: Update the test fixture’s initializeState setup around
the policiesState and managedClustersState references to mock the
useSharedAtoms() contract instead of importing or setting shared Recoil atoms
directly. Ensure the test receives the fixture values through the mocked hook
while preserving the existing policy and managed-cluster data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ea1a85b1-b21c-4ce0-b718-4c5c3a44b0e0
📒 Files selected for processing (3)
frontend/src/routes/Governance/overview/Overview.test.tsxfrontend/src/routes/Governance/overview/Overview.tsxfrontend/src/routes/Governance/overview/SecurityGroupPolicySummarySidebar.tsx
The Governance Overview Standards/Categories/Controls cards, and the
policy drill-down sidebar behind them, group policies by splitting
the policy.open-cluster-management.io/{standards,categories,controls}
annotation on ",". String.split(',') leaves a leading space on every
token after the first (e.g. "A, B".split(',') -> ["A", " B"]), so the
same logical value produces a second, distinct row whenever it isn't
first in a comma-separated list on some policies but is standalone or
first-in-list on others.
Trim each token after splitting in both useSecurityGroupViolations
(Overview.tsx) and the matching filter in
SecurityGroupPolicySummarySidebar.tsx so aggregation and drill-down
filtering key off the same normalized value regardless of annotation
formatting.
Reported in Red Hat case 04500068.
Signed-off-by: Sunny Gupta <sungupta@redhat.com>
e1da5eb to
2aa0aec
Compare
|
/cc @Randy424 |
The SecurityGroupPolicySummarySidebar drill-down applies the same comma-split-without-trim fix as useSecurityGroupViolations, but the existing regression test only exercised the Overview card path, leaving the sidebar's fixed line uncovered on new code (SonarCloud flagged 50% coverage on new code, below the 70% gate). Add a test that renders SecurityGroupPolicySummarySidebar directly with a policy whose matching standard is not first in its comma-separated annotation, and asserts the policy appears in the filtered list. Verified this test fails without the sidebar's .trim() fix and passes with it. Signed-off-by: Sunny Gupta <sungupta@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
frontend/src/routes/Governance/overview/Overview.test.tsx (1)
272-272: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winQuery the policy link by role.
The sidebar renders the policy name in a
Link; assert it withfindByRole('link', { name: 'policy-standards-last-sidebar' })rather than text lookup.As per coding guidelines, prefer role queries over text queries.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/routes/Governance/overview/Overview.test.tsx` at line 272, Update the policy assertion in the Overview test to use screen.findByRole with the link role and accessible name “policy-standards-last-sidebar” instead of findByText, while preserving the existing async and visibility assertion.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/routes/Governance/overview/Overview.test.tsx`:
- Around line 16-20: Update the imports in the Overview test to use the ~/ alias
for newly added internal module imports, and split SecurityGroupViolations and
Policy into import type declarations because they are type-only symbols.
Preserve the existing value imports and runtime behavior.
- Around line 200-204: Replace direct atom initialization in both test fixtures
with shared atom access via useSharedAtoms(). In
frontend/src/routes/Governance/overview/Overview.test.tsx:200-204, initialize
both the policy and managed-cluster fixtures through useSharedAtoms(); at
257-260, initialize the policy fixture the same way, removing direct atom
imports or usage where applicable.
---
Nitpick comments:
In `@frontend/src/routes/Governance/overview/Overview.test.tsx`:
- Line 272: Update the policy assertion in the Overview test to use
screen.findByRole with the link role and accessible name
“policy-standards-last-sidebar” instead of findByText, while preserving the
existing async and visibility assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9cdadb73-9c64-4c27-b54f-aadf59a18148
📒 Files selected for processing (3)
frontend/src/routes/Governance/overview/Overview.test.tsxfrontend/src/routes/Governance/overview/Overview.tsxfrontend/src/routes/Governance/overview/SecurityGroupPolicySummarySidebar.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/routes/Governance/overview/SecurityGroupPolicySummarySidebar.tsx
- frontend/src/routes/Governance/overview/Overview.tsx
| import GovernanceOverview, { SecurityGroupViolations } from './Overview' | ||
| import { SecurityGroupPolicySummarySidebar } from './SecurityGroupPolicySummarySidebar' | ||
| import userEvent from '@testing-library/user-event' | ||
| import { defaultContext, PluginDataContext } from '../../../lib/PluginDataContext' | ||
| import { Policy, PolicyApiVersion, PolicyKind } from '../../../resources' |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use aliases and type-only imports.
SecurityGroupViolations and Policy are type-only; the newly added internal imports are relative. Split type imports with import type and use ~/... aliases for the new internal imports.
As per coding guidelines, use ~/ shorthand and import type for type-only imports.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/routes/Governance/overview/Overview.test.tsx` around lines 16 -
20, Update the imports in the Overview test to use the ~/ alias for newly added
internal module imports, and split SecurityGroupViolations and Policy into
import type declarations because they are type-only symbols. Preserve the
existing value imports and runtime behavior.
Source: Coding guidelines
| <RecoilRoot | ||
| initializeState={(snapshot) => { | ||
| snapshot.set(policiesState, [policyWithStandardFirstInList, policyWithStandardLastInList]) | ||
| snapshot.set(managedClustersState, mockManagedClusters) | ||
| }} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use shared atom access for the new fixtures.
Both tests set policiesState (and one sets managedClustersState) through direct atom imports.
frontend/src/routes/Governance/overview/Overview.test.tsx#L200-L204: initialize the policy and cluster fixtures throughuseSharedAtoms().frontend/src/routes/Governance/overview/Overview.test.tsx#L257-L260: initialize the policy fixture throughuseSharedAtoms().
As per coding guidelines, mock Recoil atoms via useSharedAtoms() rather than direct atom imports.
📍 Affects 1 file
frontend/src/routes/Governance/overview/Overview.test.tsx#L200-L204(this comment)frontend/src/routes/Governance/overview/Overview.test.tsx#L257-L260
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/routes/Governance/overview/Overview.test.tsx` around lines 200 -
204, Replace direct atom initialization in both test fixtures with shared atom
access via useSharedAtoms(). In
frontend/src/routes/Governance/overview/Overview.test.tsx:200-204, initialize
both the policy and managed-cluster fixtures through useSharedAtoms(); at
257-260, initialize the policy fixture the same way, removing direct atom
imports or usage where applicable.
Source: Coding guidelines
|
/retest |
|
@sungupta-llm: Cannot trigger testing until a trusted user reviews the PR and leaves an 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. |
|
/ok-to-test |
|
/ok-to-test |
|
/retest-required |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Randy424, sungupta-llm 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 |
|
@Randy424 It seems its pending on 1 more test which require your approval. |
|
/ok-to-test |
|
/test unit-tests-sonarcloud |
|
@sungupta-llm it looks like we are still running into some test failures ([frontend] FAIL src/wizards/Governance/Policy/policyWizard.test.tsx (51.115 s)) ...Could you review this unit test and confirm whether this failure is related to your changes? |
|
@Randy424 No code overlap: PolicyWizard.tsx/policyWizard.test.tsx have zero references to Overview.tsx or SecurityGroupPolicySummarySidebar.tsx (the only files this PR touches besides the test). |
|
/ok-to-test |
|
/test unit-tests-sonarcloud |
|
|
/ok-to-test |
|
/retest |
|
/cherry-pick release-2.17 |
|
@Randy424: new pull request created: #6641 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. |
|
/cherry-pick release-2.16 |
|
@Randy424: new pull request created: #6642 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. |
|
/cherry-pick release-2.15 |
|
@Randy424: new pull request created: #6643 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. |
4 participants
Summary
The Governance Overview Standards/Categories/Controls cards (and the policy
drill-down sidebar behind them) group policies by splitting the
policy.open-cluster-management.io/{standards,categories,controls}annotation on
,:String.split(',')leaves a leading space on every token after the first(e.g.
"A, B".split(',')→["A", " B"]). If the same logical value (e.g."NIST SP 800-53") is the first item in one policy's annotation but a lateritem in another policy's annotation, the two policies produce different map
keys (
"NIST SP 800-53"vs" NIST SP 800-53") and render as two separaterows on the card instead of aggregating into one.
The same unfixed split exists in the drill-down sidebar filter
(
SecurityGroupPolicySummarySidebar.tsx), which does a strict equalitycheck (
name === violation.name) against the untrimmed token, so thesidebar's policy list can also fail to match policies whose annotation
formatting differs from the row that was clicked.
Fix
Trim each token after splitting, in both
useSecurityGroupViolations(
Overview.tsx) and the matching filter inSecurityGroupPolicySummarySidebar.tsx, so aggregation and drill-downfiltering key off the same normalized value regardless of comma-spacing.
Reproduction (single managed cluster is sufficient)
policy.open-cluster-management.io/standards: "NIST SP 800-53, PCI-DSS 4.0"policy.open-cluster-management.io/standards: "CIS OpenShift Benchmark, PCI-DSS 4.0, NIST SP 800-53"separate rows instead of one aggregated row.
Test plan
Overview.test.tsxthat renders the Standardscard with two policies carrying the same standard in different list
positions, and asserts a single aggregated row.
Expected: 1, Received: 2)and passes with it.
eslint --max-warnings=0clean on all changed files.Reported in Red Hat case 04500068.
Summary by CodeRabbit