ACM-30888 Remove placement feature gate#6066
Open
fxiang1 wants to merge 5 commits intostolostron:mainfrom
Open
ACM-30888 Remove placement feature gate#6066fxiang1 wants to merge 5 commits intostolostron:mainfrom
fxiang1 wants to merge 5 commits intostolostron:mainfrom
Conversation
Signed-off-by: fxiang1 <fxiang@redhat.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fxiang1 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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoves the backend ChangesConfiguration & Types
Feature-flag removal & UI behavior
Hook API change
Testing & network mocks
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
Author
|
Put on hold until QE is ready /hold |
![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
🧹 Nitpick comments (1)
frontend/src/wizards/Placement/PlacementSection.test.tsx (1)
348-349: RedundantmockSettings = {}assignments.Lines 349, 370, 392, and 448 explicitly set
mockSettings = {}, but this is already done inbeforeEach(line 89). These lines can be removed for cleaner tests.♻️ Remove redundant assignments
it('sets footer content with single placement', () => { - mockSettings = {} mockResources = [Apply similar removal to lines 370, 392, and 448.
Also applies to: 369-370, 392-392, 448-448
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/wizards/Placement/PlacementSection.test.tsx` around lines 348 - 349, Remove the redundant in-test reassignments of mockSettings (e.g., the explicit mockSettings = {} inside the test "sets footer content with single placement" and the other tests that repeat this) because beforeEach already initializes mockSettings; simply delete those assignments so tests rely on the shared beforeEach setup (look for occurrences of mockSettings = {} in PlacementSection.test.tsx and remove them).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/wizards/Placement/Placement.tsx`:
- Around line 118-119: The debug hook is being invoked unconditionally via
usePlacementDebug(placement) even when ownsDebugUI is false or
props.placementDebugState is provided; change the call to pass a guarded input
so the hook runs only when needed, e.g. const ownDebugState =
usePlacementDebug(ownsDebugUI ? placement : undefined), and keep the existing
fallback const { matched, notMatched, totalClusters, matchedCount, error } =
props.placementDebugState ?? ownDebugState so external placementDebugState still
overrides the guarded hook result.
---
Nitpick comments:
In `@frontend/src/wizards/Placement/PlacementSection.test.tsx`:
- Around line 348-349: Remove the redundant in-test reassignments of
mockSettings (e.g., the explicit mockSettings = {} inside the test "sets footer
content with single placement" and the other tests that repeat this) because
beforeEach already initializes mockSettings; simply delete those assignments so
tests rely on the shared beforeEach setup (look for occurrences of mockSettings
= {} in PlacementSection.test.tsx and remove them).
🪄 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: Enterprise
Run ID: 3ff6a85a-0d70-43d7-a2d5-15d1be60dceb
⛔ Files ignored due to path filters (1)
frontend/public/locales/en/translation.jsonis excluded by!frontend/public/locales/**
📒 Files selected for processing (23)
backend/config/enhancedPlacementfrontend/src/atoms.tsfrontend/src/routes/Applications/AdvancedConfiguration.test.tsxfrontend/src/routes/Applications/AdvancedConfiguration.tsxfrontend/src/routes/Applications/ApplicationDetails/ApplicationDetails/ApplicationDetails.test.tsxfrontend/src/routes/Applications/ApplicationDetails/ApplicationDetails/ApplicationDetails.tsxfrontend/src/routes/Applications/components/ToggleSelector.tsxfrontend/src/routes/Governance/policies/policy-details/PolicyDetailsOverview.test.tsxfrontend/src/routes/Governance/policies/policy-details/PolicyDetailsOverview.tsxfrontend/src/routes/Governance/policy-sets/components/PolicySetCard.test.tsxfrontend/src/routes/Governance/policy-sets/components/PolicySetCard.tsxfrontend/src/routes/Infrastructure/Clusters/ClustersPage.tsxfrontend/src/routes/Infrastructure/Clusters/ManagedClusters/ClusterDetails/ClusterOverview/ClusterOverview.tsxfrontend/src/routes/Infrastructure/Clusters/Placements/CreatePlacement/EditPlacement.tsxfrontend/src/routes/Infrastructure/Clusters/Placements/CreatePlacement/PlacementWizard.tsxfrontend/src/routes/Infrastructure/Clusters/Placements/PlacementDetails/PlacementDetails.test.tsxfrontend/src/routes/Infrastructure/Clusters/Placements/Placements.test.tsxfrontend/src/wizards/Argo/ArgoWizard.tsxfrontend/src/wizards/Placement/Placement.tsxfrontend/src/wizards/Placement/PlacementSection.test.tsxfrontend/src/wizards/Placement/PlacementSection.tsxfrontend/src/wizards/Placement/usePlacementDebug.test.tsfrontend/src/wizards/Placement/usePlacementDebug.ts
💤 Files with no reviewable changes (2)
- backend/config/enhancedPlacement
- frontend/src/routes/Governance/policies/policy-details/PolicyDetailsOverview.test.tsx
Signed-off-by: fxiang1 <fxiang@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/lib/nock-util.ts`:
- Around line 401-406: The helper nockIgnorePlacementDebug() is returning an
empty shape that makes usePlacementDebug() compute totalClusters = 0 and alters
placement behavior; update nockIgnorePlacementDebug (or create a scoped variant)
to return a realistic placement-debug fixture that preserves expected keys and
non-empty arrays (matching what usePlacementDebug() expects), or rename/scope
the helper and use it only in tests that never consume placement results so it
doesn't affect placement flows.
🪄 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: Enterprise
Run ID: 2f9297e8-3c02-45e2-8044-4ccafb0ed7f2
📒 Files selected for processing (10)
frontend/src/lib/nock-util.tsfrontend/src/routes/Applications/CreateArgoApplication/CreatePullApplicationSet.test.tsxfrontend/src/routes/Applications/CreateArgoApplication/CreatePushApplicationSet.test.tsxfrontend/src/routes/Governance/policies/CreatePolicy.test.tsxfrontend/src/routes/Governance/policy-sets/CreatePolicySet.test.tsxfrontend/src/routes/Governance/policy-sets/EditPolicySet.test.tsxfrontend/src/routes/Infrastructure/Clusters/ManagedClusters/ClusterDetails/ClusterOverview/ClusterOverview.test.tsxfrontend/src/wizards/Argo/ArgoWizard.test.tsxfrontend/src/wizards/Governance/PolicySet/PolicySetWizard.test.tsxfrontend/src/wizards/Placement/Placement.tsx
✅ Files skipped from review due to trivial changes (1)
- frontend/src/routes/Governance/policy-sets/EditPolicySet.test.tsx
Signed-off-by: fxiang1 <fxiang@redhat.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/src/wizards/Governance/Policy/policyWizard.test.tsx (1)
132-133: Centralize placement-debug mock setup inbeforeEachfor consistency.
nockIgnorePlacementDebug()is added in two tests, but a similar flow at Line [332] still renders without it. A suite-level setup avoids omissions and keeps test isolation consistent.♻️ Suggested refactor
describe('Policy wizard', () => { + beforeEach(() => { + nockIgnorePlacementDebug() + }) + test('can show correct cluster sets dropdown', async () => { - nockIgnorePlacementDebug() const { container } = render(<TestPolicyWizard />) @@ test('default tolerations are set when creating new placement', async () => { - nockIgnorePlacementDebug() render(<TestPolicyWizard yamlEditor={() => <WizardSyncEditor />} />)As per coding guidelines, "Ensure proper mocking and isolation."
Also applies to: 301-302
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/wizards/Governance/Policy/policyWizard.test.tsx` around lines 132 - 133, The test-suite is inconsistent in mocking placement-debug; call nockIgnorePlacementDebug() from a suite-level beforeEach so every test (including those rendering TestPolicyWizard at various places) gets the same mock setup; locate usages of nockIgnorePlacementDebug and remove the duplicated per-test calls, then add a beforeEach that invokes nockIgnorePlacementDebug so all tests (e.g., those rendering TestPolicyWizard) run with consistent placement-debug mocking and isolation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/src/wizards/Governance/Policy/policyWizard.test.tsx`:
- Around line 132-133: The test-suite is inconsistent in mocking
placement-debug; call nockIgnorePlacementDebug() from a suite-level beforeEach
so every test (including those rendering TestPolicyWizard at various places)
gets the same mock setup; locate usages of nockIgnorePlacementDebug and remove
the duplicated per-test calls, then add a beforeEach that invokes
nockIgnorePlacementDebug so all tests (e.g., those rendering TestPolicyWizard)
run with consistent placement-debug mocking and isolation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: dd904041-208d-4387-86bb-a6e2809ff5e9
📒 Files selected for processing (2)
frontend/src/wizards/Argo/ArgoWizard.test.tsxfrontend/src/wizards/Governance/Policy/policyWizard.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/wizards/Argo/ArgoWizard.test.tsx
Signed-off-by: fxiang1 <fxiang@redhat.com>
Signed-off-by: fxiang1 <fxiang@redhat.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/src/routes/Applications/AdvancedConfiguration.tsx (1)
664-678: 💤 Low valueWrap the Alert in a
StackItemfor consistent Stack gutter behavior.
<Stack hasGutter>applies its gutter viaStackItemchildren. TheApplicationDeploymentHighlightsandToggleSelectorsiblings are both wrapped inStackItem, but the Alert is a bare child. Wrapping it maintains uniform spacing.♻️ Proposed fix
- <Alert - title={t('Page deprecation')} - isInline - variant="warning" - actionLinks={ - <AlertActionLink component="a" target="_blank" rel="noreferrer" href={DOC_LINKS.DEPRECATIONS_ACM}> - {t('Learn more')} - </AlertActionLink> - } - > - <Trans - i18nKey="<bold>Deprecated:</bold> Placements are managed from the <italic>Placements</italic> tab of the <italic>Infrastructure</italic> page. Select <bold>Infrastructure</bold> > <bold>Clusters</bold> > <bold>Placements</bold>. You can also view placement details directly within individual applications or policies." - components={{ bold: <strong />, italic: <em /> }} - /> - </Alert> + <StackItem> + <Alert + title={t('Page deprecation')} + isInline + variant="warning" + actionLinks={ + <AlertActionLink component="a" target="_blank" rel="noreferrer" href={DOC_LINKS.DEPRECATIONS_ACM}> + {t('Learn more')} + </AlertActionLink> + } + > + <Trans + i18nKey="<bold>Deprecated:</bold> Placements are managed from the <italic>Placements</italic> tab of the <italic>Infrastructure</italic> page. Select <bold>Infrastructure</bold> > <bold>Clusters</bold> > <bold>Placements</bold>. You can also view placement details directly within individual applications or policies." + components={{ bold: <strong />, italic: <em /> }} + /> + </Alert> + </StackItem>🤖 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/Applications/AdvancedConfiguration.tsx` around lines 664 - 678, The Alert currently sits directly under the Stack with siblings ApplicationDeploymentHighlights and ToggleSelector wrapped in StackItem, which breaks Stack's gutter spacing; wrap the Alert JSX in a <StackItem> so it becomes a direct StackItem child (keeping the same Alert props and children) to restore consistent spacing when using <Stack hasGutter>.
🤖 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/Applications/AdvancedConfiguration.tsx`:
- Around line 664-678: The Alert currently sits directly under the Stack with
siblings ApplicationDeploymentHighlights and ToggleSelector wrapped in
StackItem, which breaks Stack's gutter spacing; wrap the Alert JSX in a
<StackItem> so it becomes a direct StackItem child (keeping the same Alert props
and children) to restore consistent spacing when using <Stack hasGutter>.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ae182f62-3022-4613-8555-b189843c45cf
⛔ Files ignored due to path filters (1)
frontend/public/locales/en/translation.jsonis excluded by!frontend/public/locales/**
📒 Files selected for processing (2)
frontend/src/routes/Applications/AdvancedConfiguration.test.tsxfrontend/src/routes/Applications/AdvancedConfiguration.tsx
✅ Files skipped from review due to trivial changes (1)
- frontend/src/routes/Applications/AdvancedConfiguration.test.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📝 Summary
Ticket Summary (Title):
Remove enhancedPlacement feature flag
Ticket Link:
https://redhat.atlassian.net/browse/ACM-30888
Type of Change:
✅ Checklist
General
ACM-12340 Fix bug with...)If Feature
If Bugfix
🗒️ Notes for Reviewers
Summary by CodeRabbit
Enhancements
Bug Fixes
Chores