OU-1107 OU-1108: ACM alerting UI with alerts and perses - #1105
OU-1107 OU-1108: ACM alerting UI with alerts and perses#1105etmurasaki wants to merge 4 commits into
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
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 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: etmurasaki 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 |
|
/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 |
|
/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
| # Generate a random filename | ||
| RANDOM_FILE="/tmp/coo_monitoring_csv_$(date +%s%N).yaml" | ||
|
|
||
| COO_CSV_NAME=$(oc get csv --kubeconfig "${KUBECONFIG}" --namespace="${MCP_NAMESPACE}" | grep "cluster-observability-operator" | awk '{print $1}') |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Guard against empty or multi-line COO_CSV_NAME.
grep "cluster-observability-operator" | awk '{print $1}' can return zero or multiple matches if two CSV versions coexist during an OLM upgrade. An empty result makes the later oc get csv "" call fail with an unclear error. A multi-line result makes COO_CSV_NAME hold more than one CSV name, which breaks the oc get csv "${COO_CSV_NAME}" calls in patch_csv and the verification loop.
Add a check after the lookup, and select a single, deterministic CSV when more than one match exists.
🔧 Proposed fix
-COO_CSV_NAME=$(oc get csv --kubeconfig "${KUBECONFIG}" --namespace="${MCP_NAMESPACE}" | grep "cluster-observability-operator" | awk '{print $1}')
+COO_CSV_NAME=$(oc get csv --kubeconfig "${KUBECONFIG}" --namespace="${MCP_NAMESPACE}" | grep "cluster-observability-operator" | awk '{print $1}' | sort -V | tail -1)
echo "COO_CSV_NAME: ${COO_CSV_NAME}"
+if [ -z "${COO_CSV_NAME}" ]; then
+ echo "ERROR: could not find cluster-observability-operator CSV in namespace ${MCP_NAMESPACE}"
+ exit 1
+fi🤖 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/fixtures/coo/update-mcp-image.sh` at 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.
| # Verify the CSV patch was applied and not reverted by OLM. | ||
| # OLM may reconcile the CSV and revert the image back to the original. | ||
| MAX_CSV_RETRIES=6 | ||
| CSV_RETRY_INTERVAL=10 | ||
| CSV_PATCHED=false | ||
|
|
||
| for i in $(seq 1 $MAX_CSV_RETRIES); do | ||
| sleep $CSV_RETRY_INTERVAL | ||
|
|
||
| LIVE_CSV=$(oc get csv "${COO_CSV_NAME}" -n "${MCP_NAMESPACE}" -o yaml --kubeconfig "${KUBECONFIG}") | ||
|
|
||
| if echo "${LIVE_CSV}" | grep -q "${MCP_CONSOLE_IMAGE}"; then | ||
| echo "CSV patch verified successfully on attempt ${i}" | ||
| CSV_PATCHED=true | ||
| break | ||
| fi | ||
|
|
||
| echo "CSV patch was reverted by OLM (attempt ${i}/${MAX_CSV_RETRIES}). Re-applying..." | ||
| patch_csv | ||
| done | ||
|
|
||
| if [ "${CSV_PATCHED}" = false ]; then | ||
| echo "WARNING: CSV patch could not be stabilized after ${MAX_CSV_RETRIES} attempts" | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find the Cypress config value for readyTimeoutMilliseconds to compare against the script's new worst-case runtime.
rg -n "readyTimeoutMilliseconds" -C3Repository: openshift/monitoring-plugin
Length of output: 165
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(update-mcp-image\.sh|image-patch-commands\.ts|cypress|config)' | head -200
printf '%s\n' '--- timeout-related references ---'
rg -n -i "readyTimeoutMilliseconds|timeout.*milliseconds|cy\.exec|patchCOOCSVImage|verifyMonitoringConsolePluginImage" --glob '!node_modules' .
printf '%s\n' '--- script ---'
cat -n web/cypress/fixtures/coo/update-mcp-image.shRepository: openshift/monitoring-plugin
Length of output: 37697
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Cypress timeout configuration ---'
cat -n web/cypress.config.ts | sed -n '52,72p'
printf '%s\n' '--- image patch commands ---'
cat -n web/cypress/support/commands/image-patch-commands.ts | sed -n '1,215p'
printf '%s\n' '--- call sites ---'
cat -n web/cypress/support/commands/operator-commands.ts | sed -n '225,255p'
printf '%s\n' '--- script timing constants and waits ---'
rg -n "sleep|oc wait|MAX_CSV_RETRIES|CSV_RETRY_INTERVAL|timeout:" web/cypress/fixtures/coo/update-mcp-image.sh web/cypress/support/commands/image-patch-commands.tsRepository: openshift/monitoring-plugin
Length of output: 12263
Increase the cy.exec timeout or reduce the script wait budget.
readyTimeoutMilliseconds is 120 seconds, but the script can spend at least 145 seconds in sleeps and oc wait, before command overhead. Both callers use this timeout. failOnNonZeroExit: false does not prevent a timeout failure.
🤖 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/fixtures/coo/update-mcp-image.sh` around lines 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.
|
|
||
| LIVE_CSV=$(oc get csv "${COO_CSV_NAME}" -n "${MCP_NAMESPACE}" -o yaml --kubeconfig "${KUBECONFIG}") | ||
|
|
||
| if echo "${LIVE_CSV}" | grep -q "${MCP_CONSOLE_IMAGE}"; then |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use literal-string matching for the image verification check.
grep -q "${MCP_CONSOLE_IMAGE}" interprets the image reference as a basic regular expression. The . characters in a typical image reference (registry host, tag) are regex metacharacters, so this check can produce a false-positive match against a similar but different image string.
Use grep -qF to match the image reference as a literal string.
🔧 Proposed fix
- if echo "${LIVE_CSV}" | grep -q "${MCP_CONSOLE_IMAGE}"; then
+ if echo "${LIVE_CSV}" | grep -qF "${MCP_CONSOLE_IMAGE}"; then📝 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.
| if echo "${LIVE_CSV}" | grep -q "${MCP_CONSOLE_IMAGE}"; then | |
| if echo "${LIVE_CSV}" | grep -qF "${MCP_CONSOLE_IMAGE}"; then |
🤖 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/fixtures/coo/update-mcp-image.sh` at 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.
| # Wait for the operator to reconcile the change and make sure all the pods are running. | ||
| sleep 25 | ||
| OUTPUT=`oc wait --for=condition=ready pods -l app.kubernetes.io/name=observability-operator -n "${MCP_NAMESPACE}" --timeout=60s --kubeconfig "${KUBECONFIG}"` | ||
| echo "${OUTPUT}" No newline at end of file | ||
| OUTPUT=$(oc wait --for=condition=ready pods -l app.kubernetes.io/name=observability-operator -n "${MCP_NAMESPACE}" --timeout=60s --kubeconfig "${KUBECONFIG}") | ||
| echo "${OUTPUT}" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check for a `set -e`/`set -euo pipefail` directive at the top of the script.
sed -n '1,3p' web/cypress/fixtures/coo/update-mcp-image.shRepository: openshift/monitoring-plugin
Length of output: 264
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- update-mcp-image.sh ---'
sed -n '1,140p' web/cypress/fixtures/coo/update-mcp-image.sh
printf '%s\n' '--- references ---'
rg -n -C 5 'update-mcp-image\.sh|patchCOOCSVImage|cy\.exec' web/cypressRepository: openshift/monitoring-plugin
Length of output: 50384
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- patchCOOCSVImage and script invocation ---'
rg -n -C 12 'patchCOOCSVImage|update-mcp-image\.sh|MCP_CONSOLE_IMAGE' web/cypress/support web/cypress/e2e web/cypress --glob '*.ts' --glob '*.js' --glob '*.sh' | head -n 240
printf '%s\n' '--- direct wait-status behavior ---'
bash -c 'tmp=$(mktemp); printf "%s\n" "#!/bin/bash" "OUTPUT=\$(false)" "echo \"\${OUTPUT}\"" > "$tmp"; chmod +x "$tmp"; "$tmp"; printf "exit=%s\n" "$?"; rm -f "$tmp"'Repository: openshift/monitoring-plugin
Length of output: 20329
Propagate oc wait failures from update-mcp-image.sh.
The script has no set -e or set -euo pipefail. A failed or timed-out oc wait is followed by a successful echo, so the script returns exit code 0. Check the wait status explicitly and exit non-zero when the operator pods are not ready.
🤖 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/fixtures/coo/update-mcp-image.sh` around lines 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.
| const checkAndFix = (attempt: number): void => { | ||
| cy.exec( | ||
| `oc get pods -l app.kubernetes.io/instance=monitoring -n ${MCP.namespace} ` + | ||
| `-o jsonpath='{.items[0].spec.containers[0].image}' --kubeconfig "${kubeconfig}"`, | ||
| { failOnNonZeroExit: false }, | ||
| ).then((result) => { | ||
| const currentImage = result.stdout.replace(/'/g, '').trim(); | ||
| cy.log( | ||
| `monitoring-console-plugin image check ` + | ||
| `(attempt ${attempt}/${maxAttempts}): ${currentImage}`, | ||
| ); | ||
|
|
||
| if (currentImage === expectedImage) { | ||
| cy.log('monitoring-console-plugin pod image verified successfully'); | ||
| return; | ||
| } | ||
|
|
||
| if (attempt >= maxAttempts) { | ||
| throw new Error( | ||
| `monitoring-console-plugin pod image mismatch after ${maxAttempts} attempts.\n` + | ||
| `Expected: ${expectedImage}\nActual: ${currentImage}`, | ||
| ); | ||
| } | ||
|
|
||
| cy.log('Image mismatch detected. Re-patching CSV and deployment...'); | ||
|
|
||
| // Re-run the CSV patch script | ||
| cy.exec('./cypress/fixtures/coo/update-mcp-image.sh', { | ||
| env: { | ||
| MCP_CONSOLE_IMAGE: expectedImage, | ||
| KUBECONFIG: kubeconfig, | ||
| MCP_NAMESPACE: MCP.namespace, | ||
| }, | ||
| timeout: readyTimeoutMilliseconds, | ||
| failOnNonZeroExit: false, | ||
| }); | ||
|
|
||
| // Patch the deployment directly in case the operator doesn't reconcile | ||
| cy.exec( | ||
| `oc get deployment -l app.kubernetes.io/instance=monitoring -n ${MCP.namespace} ` + | ||
| `-o jsonpath='{.items[0].metadata.name}' --kubeconfig "${kubeconfig}"`, | ||
| { failOnNonZeroExit: false }, | ||
| ).then((deployResult) => { | ||
| const deployName = deployResult.stdout.replace(/'/g, '').trim(); | ||
| if (deployName) { | ||
| cy.log(`Patching deployment/${deployName} image directly`); | ||
| cy.exec( | ||
| `oc patch deployment ${deployName} -n ${MCP.namespace} --type=json ` + | ||
| `-p '[{"op":"replace","path":"/spec/template/spec/containers/0/image",` + | ||
| `"value":"${expectedImage}"}]' ` + | ||
| `--kubeconfig "${kubeconfig}"`, | ||
| ); | ||
| cy.exec( | ||
| `oc rollout status deployment/${deployName} -n ${MCP.namespace} ` + | ||
| `--timeout=120s --kubeconfig "${kubeconfig}"`, | ||
| { timeout: 130000 }, | ||
| ); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
file='web/cypress/support/commands/image-patch-commands.ts'
printf '%s\n' '--- target file ---'
sed -n '1,190p' "$file"
printf '%s\n' '--- related definitions and call sites ---'
rg -n -C 3 'checkAndFix|expectedImage|patchCOOCSVImage|MCP\.namespace|image-patch-commands' web/cypress web --glob '*.{ts,tsx,js,sh}' | head -n 500
printf '%s\n' '--- fixture script ---'
script='web/cypress/fixtures/coo/update-mcp-image.sh'
if [ -f "$script" ]; then
sed -n '1,220p' "$script"
fiRepository: openshift/monitoring-plugin
Length of output: 50383
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- relevant environment definitions and call sites ---'
rg -n -C 4 \
'MCP_CONSOLE_IMAGE|KUBECONFIG_PATH|namespace:|verifyMonitoringConsolePluginImage|setupMonitoringConsolePlugin' \
web/cypress --glob '*.{ts,tsx,js,json,yaml,yml,sh}' \
| grep -E 'MCP_CONSOLE_IMAGE|KUBECONFIG_PATH|verifyMonitoringConsolePluginImage|setupMonitoringConsolePlugin|namespace:' \
| head -n 300
printf '%s\n' '--- image update script ---'
script='web/cypress/fixtures/coo/update-mcp-image.sh'
wc -l "$script"
cat -n "$script"
printf '%s\n' '--- Cypress exec implementation references ---'
rg -n -C 3 'cy\.exec|exec\(' web/cypress package.json cypress.config.* .github 2>/dev/null \
| head -n 250
printf '%s\n' '--- shell parsing probe ---'
python3 - <<'PY'
import subprocess
contexts = {
"namespace_get_pods": (
'printf "%s\\n" oc get pods -l app.kubernetes.io/instance=monitoring '
'-n {value} -o jsonpath=\'{{.items[0].spec.containers[0].image}}\' '
'--kubeconfig "/tmp/kubeconfig"'
),
"kubeconfig_get_pods": (
'printf "%s\\n" oc get pods -n safe '
'-o jsonpath=\'{{.items[0].spec.containers[0].image}}\' '
'--kubeconfig "{value}"'
),
"deploy_patch": (
'printf "%s\\n" oc patch deployment {deploy} -n safe --type=json '
'-p \'[{{"op":"replace","path":"/spec/template/spec/containers/0/image",'
'"value":"{image}"}}]\' --kubeconfig "/tmp/kubeconfig"'
),
}
values = {
"safe_namespace": "monitoring",
"bad_namespace": 'safe; printf INJECTED',
"safe_kubeconfig": "/tmp/kubeconfig",
"bad_kubeconfig": '/tmp/kube"; printf INJECTED; #',
"safe_deploy": "monitoring-console-plugin",
"bad_deploy": 'safe; printf INJECTED',
"safe_image": "quay.io/example/console@sha256:" + "a" * 64,
"bad_image": 'quay.io/example/console:tag\' ; printf INJECTED; #',
}
for name, template in contexts.items():
print(f"--- {name} ---")
cases = {
"safe": template.format(
value=values["safe_namespace"] if "namespace" in name
else values["safe_kubeconfig"] if "kubeconfig" in name
else values["safe_deploy"],
deploy=values["safe_deploy"],
image=values["safe_image"],
),
"malicious": template.format(
value=values["bad_namespace"] if "namespace" in name
else values["bad_kubeconfig"] if "kubeconfig" in name
else values["bad_deploy"],
deploy=values["bad_deploy"],
image=values["bad_image"],
),
}
for label, command in cases.items():
result = subprocess.run(["bash", "-c", command], text=True,
capture_output=True, check=False)
print(label, "stdout:", repr(result.stdout), "stderr:", repr(result.stderr),
"code:", result.returncode)
PYRepository: openshift/monitoring-plugin
Length of output: 50383
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- configure-env.sh relevant sections ---'
sed -n '190,235p' web/cypress/configure-env.sh
sed -n '255,300p' web/cypress/configure-env.sh
sed -n '365,390p' web/cypress/configure-env.sh
sed -n '430,455p' web/cypress/configure-env.sh
printf '%s\n' '--- exact image verification call sites ---'
rg -n -C 8 'verifyMonitoringConsolePluginImage|setupMonitoringConsolePlugin' \
web/cypress/e2e web/cypress/support --glob '*.{ts,tsx}' | head -n 220
printf '%s\n' '--- concise shell parsing probe ---'
python3 - <<'PY'
import subprocess
def run(label, command):
r = subprocess.run(["bash", "-c", command], text=True,
capture_output=True, check=False)
print(label, "=>", repr(r.stdout), "code=", r.returncode,
"stderr=", repr(r.stderr))
run("namespace-safe",
"printf '%s\\n' oc get pods -n monitoring --kubeconfig '/tmp/kubeconfig'")
run("namespace-injection",
"printf '%s\\n' oc get pods -n safe; printf '%s\\n' INJECTED --kubeconfig '/tmp/kubeconfig'")
run("kubeconfig-safe",
'printf "%s\\n" oc get pods --kubeconfig "/tmp/kubeconfig"')
run("kubeconfig-injection",
'printf "%s\\n" oc get pods --kubeconfig "/tmp/kube"; printf "%s\\n" INJECTED; # "')
run("deployment-safe",
"printf '%s\\n' oc patch deployment monitoring-console-plugin -n safe")
run("deployment-injection",
"printf '%s\\n' oc patch deployment safe; printf '%s\\n' INJECTED -n safe")
run("image-safe",
"""printf '%s\\n' 'oc patch -p '[{"op":"replace","value":"quay.io/example/plugin:tag"}]''""")
run("image-single-quote",
"""printf '%s\\n' 'oc patch -p '[{"op":"replace","value":"safe'"'"'; printf "%s\\n" INJECTED; #"}]''""")
PYRepository: openshift/monitoring-plugin
Length of output: 12491
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- MCP construction and type declarations ---'
rg -n -C 10 \
'const MCP|let MCP|MCP\s*=|COO_NAMESPACE|namespace:\s*Cypress\.env|interface.*MCP|type.*MCP' \
web/cypress --glob '*.{ts,tsx}' | head -n 260
printf '%s\n' '--- workflow and script sources for relevant environment values ---'
rg -n -C 6 \
'CYPRESS_(MCP_CONSOLE_IMAGE|COO_NAMESPACE|KUBECONFIG_PATH)|MCP_CONSOLE_IMAGE|COO_NAMESPACE|KUBECONFIG_PATH' \
.github web --glob '*.{yml,yaml,sh,ts,tsx,json}' \
| grep -vE 'fixtures/incident-scenarios|fixtures/virtualization|fixtures/coo/logging' \
| head -n 300
printf '%s\n' '--- exact target command strings ---'
sed -n '96,158p' web/cypress/support/commands/image-patch-commands.ts
printf '%s\n' '--- Kubernetes-style allow-list probe ---'
python3 - <<'PY'
import re
patterns = {
"namespace_or_deployment_dns_label": re.compile(r"^[a-z0-9]([-a-z0-9]*[a-z0-9])?$"),
"dns_subdomain": re.compile(r"^[a-z0-9]([-a-z0-9.]*[a-z0-9])?$"),
}
values = [
"openshift-cluster-observability-operator",
"monitoring-console-plugin",
"safe; printf INJECTED",
"name with spaces",
"name'withquote",
]
for value in values:
print(repr(value), {
name: bool(pattern.fullmatch(value))
for name, pattern in patterns.items()
})
PYRepository: openshift/monitoring-plugin
Length of output: 49043
Remove shell interpolation of runtime values.
MCP.namespace, deployName, expectedImage, and kubeconfig can alter these cy.exec commands. Use static command strings with quoted environment-variable expansions, or reject values that fail Kubernetes name and image-reference allow-lists before execution. Pass kubeconfig through env.KUBECONFIG instead of interpolating it.
🧰 Tools
🪛 OpenGrep (1.26.0)
[ERROR] 102-106: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 139-143: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 147-152: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 153-157: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 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 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`.
Sources: Path instructions, Linters/SAST tools
|
/test e2e-perses |
|
/test e2e-alerts |
|
/test e2e-virtualization |
|
/test e2e-coo |
|
/test e2e-perses |
|
@etmurasaki: The following tests 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. |
2 participants
Summary by CodeRabbit
Bug Fixes
Enhancements