OU-1107 OU-1108: ACM alerting UI with alerts and perses - #1105
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@etmurasaki: This pull request references OU-1107 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 task to target the "5.0.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
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
approved
|
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:
WalkthroughCypress coverage now supports Core platform and Fleet management perspectives for alerting and Perses dashboards. Fixtures enable Incidents and Perses. Shared helpers use perspective-specific alert data, configurable dashboard labels, updated selectors, readiness checks, and monitoring console image verification. ChangesMonitoring perspective coverage
Perses perspective coverage
Estimated code review effort: 4 (Complex) | ~60 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ 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
🧹 Nitpick comments (1)
web/cypress/support/monitoring/01.reg_alerts.cy.ts (1)
43-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNear-duplicate "Alerting Rules page - Filtering" test between Fleet Management and Core Platform.
testAlertsFleetManagementRegression's seconditblock (lines 73-112) andtestAlertsCorePlatformHeaderRegression's seconditblock (lines 142-182) are identical apart from one extralistPage.filter.removeIndividualTag(Source.PLATFORM);call (line 146). Extracting the shared filtering flow into a single helper (parameterized by the one extra teardown step) would prevent the two copies from drifting apart over time.♻️ Suggested extraction
function testAlertingRulesFilteringRegression( perspective: PerspectiveConfig, beforeFilters?: () => void, ) { it(`${perspective.name} perspective - Alerting > Alerting Rules page - Filtering`, () => { cy.log('2.1 use sidebar nav to go to Observe > Alerting'); nav.tabs.switchTab('Alerting rules'); alertingRuleListPage.shouldBeLoaded(); beforeFilters?.(); cy.log('2.2 clear all filters, verify filters and tags'); // ...shared body... alertingRuleListPage.filter.assertNoClearAllFilters(); }); }Then call it with
() => listPage.filter.removeIndividualTag(Source.PLATFORM)fromtestAlertsCorePlatformHeaderRegressionand with no callback fromtestAlertsFleetManagementRegression.Also applies to: 142-183
🤖 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 `@web/cypress/support/monitoring/01.reg_alerts.cy.ts` around lines 43 - 113, Extract the duplicated Alerting Rules filtering test flow from testAlertsFleetManagementRegression and testAlertsCorePlatformHeaderRegression into a shared testAlertingRulesFilteringRegression helper. Parameterize the helper with an optional beforeFilters callback, invoke it before applying filters, and pass the Source.PLATFORM removal callback only from the Core Platform test while leaving the Fleet Management call without one.
🤖 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 `@web/cypress/e2e/coo/02.acm_alerting_ui.cy.ts`:
- Around line 58-69: Add cy.closeOnboardingModalIfPresent() after
cy.switchPerspective('Fleet management') in the “Visual validation for features
that should not be displayed under Fleet management perspective” test, before
navigating with nav.sidenav.clickNavLink(...), so the test works independently
when the onboarding modal is present.
---
Nitpick comments:
In `@web/cypress/support/monitoring/01.reg_alerts.cy.ts`:
- Around line 43-113: Extract the duplicated Alerting Rules filtering test flow
from testAlertsFleetManagementRegression and
testAlertsCorePlatformHeaderRegression into a shared
testAlertingRulesFilteringRegression helper. Parameterize the helper with an
optional beforeFilters callback, invoke it before applying filters, and pass the
Source.PLATFORM removal callback only from the Core Platform test while leaving
the Fleet Management call without one.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 4b8b056f-70a6-4cd9-8420-8be26cad4f42
📒 Files selected for processing (12)
web/cypress/e2e/coo/02.acm_alerting_ui.cy.tsweb/cypress/e2e/monitoring/regression/01.reg_alerts_admin.cy.tsweb/cypress/fixtures/coo/acm-install.shweb/cypress/fixtures/monitoring/constants.tsweb/cypress/support/commands/utility-commands.tsweb/cypress/support/monitoring/01.reg_alerts.cy.tsweb/cypress/support/monitoring/04.reg_alerts_namespace.cy.tsweb/cypress/views/alerting-rule-list-page.tsweb/cypress/views/incidents-page.tsweb/cypress/views/list-page.tsweb/cypress/views/nav.tsweb/cypress/views/silences-list-page.ts
💤 Files with no reviewable changes (1)
- web/cypress/views/silences-list-page.ts
|
/test e2e-monitoring |
etmurasaki
changed the title
|
@etmurasaki: This pull request references OU-1107 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 task to target the "5.0.0" version, but no target version was set. This pull request references OU-1108 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 task to target the "5.0.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. |
|
/test e2e-monitoring |
|
/test e2e-alerts |
|
/test e2e-coo |
|
/test e2e-perses |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
web/cypress/views/list-page.ts (1)
109-114: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winKeep the optional clear-all lookup retriable.
cy.get('body').then(...)performs a single DOM check. IfDataViewToolbar-clear-all-filtersrenders after the callback,clearAllFiltersreturns without clearing active filters. Wait for a settled DataView state before the optional lookup, or use a bounded retry that treats a settled no-filter state as the absent case.🤖 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 `@web/cypress/views/list-page.ts` around lines 109 - 114, Update clearAllFilters around the DataViewToolbar-clear-all-filters lookup so it remains retriable until the DataView settles, while allowing a settled state with no clear-all control to complete successfully. Avoid relying on the single body snapshot from cy.get('body').then(...); preserve the existing behavior of clicking the first matching control when it appears.
🤖 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 `@web/cypress/e2e/coo/02.acm_perses.cy.ts`:
- Around line 22-28: Add a beforeEach hook alongside the existing before hook
that switches to the Fleet management perspective and navigates through Observe
> Dashboards before every BVT case, using the existing switchPerspective and
nav.sidenav helpers. Keep the one-time ACM setup and dashboard cleanup in before
unchanged.
In `@web/cypress/e2e/monitoring/regression/01.reg_alerts_admin.cy.ts`:
- Line 18: Update the suite metadata in the test file to match the Core platform
perspective being used. Change the describe block title and the comment text
around lines 31-33 from references to Administrator to Core platform, so the
Cypress reports and test labels accurately reflect that the test executes
cy.switchPerspective('Core platform') rather than an Administrator perspective
test.
In `@web/cypress/e2e/perses/00.coo_bvt_perses_admin.cy.ts`:
- Line 38: Rename the `describe` suite title in the Perses admin Cypress spec
from `Administrator perspective` to `Core platform` so CI reports match the
configured perspective and generated test names.
In `@web/cypress/support/perses/01.coo_list_perses_admin.cy.ts`:
- Line 28: Update every shouldBeLoaded call in the test file to pass
perspective.dashboardsPageName, matching the existing first scenario, so each
list-load assertion uses the configured page name.
In `@web/cypress/support/perses/99.coo_rbac_perses_user5.cy.ts`:
- Line 19: Remove the unused perspective fields dashboardsPageName and
dashboardSearch from the relevant test data declarations in
web/cypress/support/perses/99.coo_rbac_perses_user5.cy.ts:19-19 and
web/cypress/support/perses/99.coo_rbac_perses_user6.cy.ts:6-6, unless you
instead wire both fields into their corresponding tests.
---
Nitpick comments:
In `@web/cypress/views/list-page.ts`:
- Around line 109-114: Update clearAllFilters around the
DataViewToolbar-clear-all-filters lookup so it remains retriable until the
DataView settles, while allowing a settled state with no clear-all control to
complete successfully. Avoid relying on the single body snapshot from
cy.get('body').then(...); preserve the existing behavior of clicking the first
matching control when it appears.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: bf92b12a-d8ac-4a94-a665-fa5dcc34654f
📒 Files selected for processing (40)
web/cypress/e2e/coo/02.acm_alerting_ui.cy.tsweb/cypress/e2e/coo/02.acm_perses.cy.tsweb/cypress/e2e/monitoring/regression/01.reg_alerts_admin.cy.tsweb/cypress/e2e/perses/00.coo_bvt_perses_admin.cy.tsweb/cypress/e2e/perses/01.coo_list_perses_admin.cy.tsweb/cypress/e2e/perses/02.coo_edit_perses_admin.cy.tsweb/cypress/e2e/perses/03.coo_create_perses_admin.cy.tsweb/cypress/e2e/perses/04.coo_import_perses_admin.cy.tsweb/cypress/e2e/perses/99.coo_rbac_perses_user1.cy.tsweb/cypress/e2e/perses/99.coo_rbac_perses_user2.cy.tsweb/cypress/e2e/perses/99.coo_rbac_perses_user3.cy.tsweb/cypress/e2e/perses/99.coo_rbac_perses_user4.cy.tsweb/cypress/e2e/perses/99.coo_rbac_perses_user5.cy.tsweb/cypress/fixtures/coo/acm-alerrule-test.yamlweb/cypress/fixtures/coo/acm-install.shweb/cypress/fixtures/coo/acm-uiplugin.yamlweb/cypress/support/commands/auth-commands.tsweb/cypress/support/commands/operator-commands.tsweb/cypress/support/commands/utility-commands.tsweb/cypress/support/monitoring/01.reg_alerts.cy.tsweb/cypress/support/perses/00.coo_bvt_perses_admin.cy.tsweb/cypress/support/perses/01.coo_list_perses_admin.cy.tsweb/cypress/support/perses/01.coo_list_perses_admin_namespace.cy.tsweb/cypress/support/perses/02.coo_edit_perses_admin.cy.tsweb/cypress/support/perses/02.coo_edit_perses_admin_1.cy.tsweb/cypress/support/perses/03.coo_create_perses_admin.cy.tsweb/cypress/support/perses/04.coo_import_perses_admin.cy.tsweb/cypress/support/perses/05.coo_create_import_perses_admin.cy.tsweb/cypress/support/perses/99.coo_rbac_perses_user1.cy.tsweb/cypress/support/perses/99.coo_rbac_perses_user2.cy.tsweb/cypress/support/perses/99.coo_rbac_perses_user3.cy.tsweb/cypress/support/perses/99.coo_rbac_perses_user4.cy.tsweb/cypress/support/perses/99.coo_rbac_perses_user5.cy.tsweb/cypress/support/perses/99.coo_rbac_perses_user6.cy.tsweb/cypress/views/list-page.tsweb/cypress/views/nav.tsweb/cypress/views/perses-dashboards-edit-variables.tsweb/cypress/views/perses-dashboards-list-dashboards.tsweb/cypress/views/silence-alert-page.tsweb/src/shared/constants/data-test.ts
💤 Files with no reviewable changes (2)
- web/cypress/support/commands/operator-commands.ts
- web/src/shared/constants/data-test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- web/cypress/support/commands/utility-commands.ts
- web/cypress/e2e/coo/02.acm_alerting_ui.cy.ts
- web/cypress/support/monitoring/01.reg_alerts.cy.ts
- web/cypress/views/nav.ts
ad6b4e9 to
bbeac96
Compare
|
/test e2e-perses |
|
/test e2e-coo |
1 similar comment
|
/test e2e-coo |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
web/cypress/support/commands/image-patch-commands.ts (1)
125-168: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffReconsider re-invoking the full patch script inside the outer retry loop.
On a mismatch,
checkAndFixre-runs the entireupdate-mcp-image.shscript — which now has its own internal 6-attempt CSV retry loop and a fixed wait — and then separately patches the deployment and waits for rollout and pod readiness again. WithmaxAttempts = 5outer retries, worst-case this compounds the shell script's own retries, multiplying total CI runtime and API calls.Consider having the script perform only the CSV patch (skip its own polling loop) when invoked from this retry wrapper, since
checkAndFixalready implements outer retry and readiness waiting.🤖 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 `@web/cypress/support/commands/image-patch-commands.ts` around lines 125 - 168, Update the retry flow in checkAndFix so invoking update-mcp-image.sh performs only the CSV patch without its internal polling/retry loop when called from this wrapper. Pass the appropriate script option or environment flag, while preserving the existing deployment patch, rollout, pod-readiness checks, and outer retry behavior.
🤖 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 `@web/cypress/fixtures/coo/update-mcp-image.sh`:
- Line 44: Update the image verification check in the shell conditional around
LIVE_CSV and MCP_CONSOLE_IMAGE to use grep’s fixed-string option, ensuring the
image reference is matched literally rather than interpreted as a regular
expression.
- Line 9: Update the COO_CSV_NAME lookup in update-mcp-image.sh to validate that
at least one CSV was found and fail with a clear error if none exists. When
multiple matches are returned during an upgrade, select one deterministic CSV
name before patch_csv and the verification loop use it, ensuring COO_CSV_NAME is
always a single line.
- Around line 64-67: Update the oc wait handling in update-mcp-image.sh to
capture its exit status and explicitly exit non-zero when waiting for the
observability-operator pods fails or times out. Ensure the subsequent echo does
not mask the failure, while preserving output for successful waits.
- Around line 33-56: Reduce the total wait budget in the update-MCP-image
script, especially the CSV verification loop around MAX_CSV_RETRIES and
CSV_RETRY_INTERVAL, so it completes within the callers’ 120-second cy.exec
readyTimeoutMilliseconds, accounting for the existing oc wait and command
overhead. Preserve the retry and verification behavior while ensuring the
maximum runtime stays below the timeout.
In `@web/cypress/support/commands/image-patch-commands.ts`:
- Around line 101-159: Update the `checkAndFix` command execution flow to
eliminate shell interpolation of `MCP.namespace`, `deployName`, `expectedImage`,
and `kubeconfig`. Use static command strings with quoted environment-variable
expansions, pass `kubeconfig` via `env.KUBECONFIG`, and validate dynamic
Kubernetes names and image references against appropriate allow-lists before
invoking `cy.exec`.
---
Nitpick comments:
In `@web/cypress/support/commands/image-patch-commands.ts`:
- Around line 125-168: Update the retry flow in checkAndFix so invoking
update-mcp-image.sh performs only the CSV patch without its internal
polling/retry loop when called from this wrapper. Pass the appropriate script
option or environment flag, while preserving the existing deployment patch,
rollout, pod-readiness checks, and outer retry behavior.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 40146f88-9b03-41cb-bb57-246774ff3131
📒 Files selected for processing (3)
web/cypress/fixtures/coo/update-mcp-image.shweb/cypress/support/commands/image-patch-commands.tsweb/cypress/support/commands/operator-commands.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- web/cypress/support/commands/operator-commands.ts
|
/test e2e-perses |
|
/test e2e-alerts |
|
/test e2e-virtualization |
|
/test e2e-coo |
|
/test e2e-perses |
|
/test e2e-coo |
1 similar comment
|
/test e2e-coo |
|
/test e2e-alerts |
|
/test e2e-monitoring |
|
/test e2e-coo |
|
/test e2e-coo |
|
/test e2e-perses |
|
/override ci/prow/e2e-coo |
|
@etmurasaki: Overrode contexts on behalf of etmurasaki: ci/prow/e2e-coo 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 qe-approved |
openshift-ci
Bot
added
the
qe-approved
|
/test ? |
|
/test e2e-alerts |
|
@etmurasaki: The following test failed, say
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. |
|
/hold for branch |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/lgtm |
|
Scheduling required tests: |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: etmurasaki, PeterYurkovich 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 |
|
/override ci/prow/e2e-alerts |
|
/label qe-approved |
|
@etmurasaki: Overrode contexts on behalf of etmurasaki: ci/prow/e2e-alerts 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. |
|
/override ci/prow/e2e-management-api |
|
@etmurasaki: Overrode contexts on behalf of etmurasaki: ci/prow/e2e-management-api 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. |
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/cherry-pick release-5.0 |
|
@etmurasaki: once the present PR merges, I will cherry-pick it on top of 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. |
|
@etmurasaki: new pull request created: #1143 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 by CodeRabbit
Bug Fixes
Enhancements