NO-JIRA: Refactor cypress setup and operator commands#788
Open
DavidRajnoha wants to merge 5 commits intoopenshift:mainfrom
Open
NO-JIRA: Refactor cypress setup and operator commands#788DavidRajnoha wants to merge 5 commits intoopenshift:mainfrom
DavidRajnoha wants to merge 5 commits intoopenshift:mainfrom
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
openshift-merge-robot
added
the
needs-rebase
DavidRajnoha
changed the title
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: DavidRajnoha The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
DavidRajnoha
force-pushed
the
refactor-cypress-setup-and-operator-commands
branch
2 times, most recently
from
842cb49 to
1b21132
Compare
…d gating Prompts now flow in logical order: Cluster Connection → Installation Mode → Operators → Runtime. Irrelevant questions are hidden based on earlier answers (e.g. skip-all hides all operator questions, skip-COO hides COO images, catalog-install hides bundle image prompts). Kubeconfig defaults to ~/.kube/config so oc login is sufficient. Extracted pick_kubeconfig_interactive to deduplicate ~120 lines of Downloads scanning logic. configure-env.sh fixes: - Fix `[[ ]] && assignment` patterns that crash under set -euo pipefail when the test is false (exit code 1 triggers set -e) - Add zsh compatibility: when sourced from zsh, delegates interactive prompts to bash then auto-sources the generated export-env.sh - Add prominent message in non-sourced mode about needing to source Co-authored-by: Cursor <cursoragent@cursor.com>
…ional COO setup
Split the monolithic operator-commands.ts (932 lines) into focused modules:
- auth-commands.ts: expanded with operatorAuthUtils (RBAC, OAuth, session keys)
- coo-install-commands.ts: COO install/uninstall/namespace lifecycle
- image-patch-commands.ts: CSV image patching utilities
- dashboards-commands.ts: Perses dashboards, troubleshooting panel, UIPlugin CR
operator-commands.ts is now a slim orchestrator (~280 lines) that composes
the above modules into Cypress commands.
Added COOSetupOptions interface so callers can skip unnecessary setup steps:
- dashboards (Perses dashboards, perses pod, health-analyzer ServiceMonitor)
- troubleshootingPanel (korrel8r pod)
- healthAnalyzer (CHA image patch)
All incident tests now use { dashboards: false, troubleshootingPanel: false,
healthAnalyzer: false } to skip Perses/korrel8r/CHA setup they don't need.
The Monitoring UIPlugin CR creation is extracted into its own function and
always runs, since it's required by all COO features including incidents.
Co-authored-by: Cursor <cursoragent@cursor.com>
Setup verification improvements: - Replace all `sleep N && oc wait` patterns with cy.waitUntil-based polling via shared wait-utils.ts helpers (waitForPodsReady, waitForPodsReadyOrAbsent, waitForResourceCondition) - Make UI verification in waitForCOOReady conditional on COO_UI_INSTALL since CLI readiness check is sufficient for bundle installs - Eliminates ~195s of unconditional sleeping in a full COO setup Co-authored-by: Cursor <cursoragent@cursor.com>
DavidRajnoha
force-pushed
the
refactor-cypress-setup-and-operator-commands
branch
from
1b21132 to
f939493
Compare
openshift-merge-robot
removed
the
needs-rebase
DavidRajnoha
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
DavidRajnoha
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@DavidRajnoha: This pull request explicitly references no jira issue. 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. |
📝 WalkthroughWalkthroughRewrites the Cypress environment configurator for better shell compatibility and modular exports; adds multiple new Cypress support utilities (auth, COO install, dashboards, image patching, wait helpers); refactors operator commands to use these utilities; and updates incident tests to pass new options to cy.beforeBlockCOO. Changes
Sequence DiagramssequenceDiagram
participant Test as Test Suite
participant beforeBlockCOO as beforeBlockCOO
participant cooInstallUtils as cooInstallUtils
participant imagePatchUtils as imagePatchUtils
participant dashboardsUtils as dashboardsUtils
participant Kubernetes as Kubernetes Cluster
participant UI as Test UI
Test->>beforeBlockCOO: beforeBlockCOO(MCP, MP, options)
beforeBlockCOO->>cooInstallUtils: installCOO(MCP)
cooInstallUtils->>Kubernetes: apply manifests / wait for pods
Kubernetes-->>cooInstallUtils: pods ready
alt options.dashboards
beforeBlockCOO->>dashboardsUtils: setupDashboardsAndPlugins(MCP)
dashboardsUtils->>Kubernetes: create namespaces & apply manifests
Kubernetes-->>dashboardsUtils: dashboard pods ready
dashboardsUtils->>UI: navigate & verify dashboards
end
alt options.troubleshootingPanel
beforeBlockCOO->>dashboardsUtils: setupTroubleshootingPanel(MCP)
dashboardsUtils->>Kubernetes: deploy troubleshooting plugin
Kubernetes-->>dashboardsUtils: plugin pods ready
end
alt MP image override present
beforeBlockCOO->>imagePatchUtils: setupMonitoringPluginImage(MP)
imagePatchUtils->>Kubernetes: patch CSV / wait for pods
Kubernetes-->>imagePatchUtils: pods ready
end
beforeBlockCOO-->>Test: setup complete
sequenceDiagram
participant Test as Test Suite
participant authUtils as operatorAuthUtils
participant RBAC as RBAC Config
participant OAuth as OAuth Discovery
participant LoginUI as Login UI
participant Kubernetes as Kubernetes
Test->>authUtils: performLoginAndAuth(useSession)
authUtils->>RBAC: configure cluster RBAC
RBAC->>Kubernetes: apply rolebindings
Kubernetes-->>RBAC: done
authUtils->>OAuth: fetch OAuth client redirect URL
OAuth-->>authUtils: return origin
alt useSession
authUtils->>authUtils: generateCOOSessionKey(...)
authUtils->>LoginUI: login with session
else
authUtils->>LoginUI: login without session
end
LoginUI-->>authUtils: login success
authUtils-->>Test: auth complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/cypress/e2e/incidents/01.incidents.cy.ts (1)
98-98:⚠️ Potential issue | 🟡 MinorPre-existing bug: missing function call parentheses.
incidentsPage.clearAllFiltersis a property reference (no-op), not a function invocation. This means filters are not actually cleared before step 5.3, which could make the test order-dependent.- incidentsPage.clearAllFilters + incidentsPage.clearAllFilters();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/e2e/incidents/01.incidents.cy.ts` at line 98, The test is referencing incidentsPage.clearAllFilters as a property instead of invoking it, so filters aren't being cleared; change the reference to a function call by invoking incidentsPage.clearAllFilters() wherever it's used (e.g., the occurrence in the test at the given diff) so the method executes and clears filters before step 5.3.
🧹 Nitpick comments (3)
web/cypress/support/commands/dashboards-commands.ts (1)
24-58: Refactor duplicated manifest apply/delete paths.The COO 1.2.0 vs 1.4.0 branches repeat the same manifest list twice for setup and cleanup, which increases drift risk.
🧹 Suggested refactor pattern
+ const dashboardsBasePath = Cypress.env('COO_UI_INSTALL') + ? './cypress/fixtures/coo/coo121_perses/dashboards' + : './cypress/fixtures/coo/coo141_perses/dashboards'; + + const manifests = [ + 'openshift-cluster-sample-dashboard.yaml', + 'perses-dashboard-sample.yaml', + 'prometheus-overview-variables.yaml', + 'thanos-compact-overview-1var.yaml', + 'thanos-querier-datasource.yaml', + ]; + + manifests.forEach((file) => { + cy.exec(`oc apply -f ${dashboardsBasePath}/${file} --kubeconfig ${Cypress.env('KUBECONFIG_PATH')}`); + });And similarly in cleanup:
+ manifests.forEach((file) => { + cy.executeAndDelete(`oc delete -f ${dashboardsBasePath}/${file} --ignore-not-found --kubeconfig ${Cypress.env('KUBECONFIG_PATH')}`); + });Also applies to: 111-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/support/commands/dashboards-commands.ts` around lines 24 - 58, The two branches gated by Cypress.env('COO_UI_INSTALL') duplicate the same list of cy.exec manifest applies (the multiple cy.exec lines in the block guarded by COO_UI_INSTALL and its else), causing drift; replace the duplicated blocks by computing a baseFolder variable (e.g., coo121_perses vs coo141_perses) based on Cypress.env('COO_UI_INSTALL'), store the manifest filenames in a single array (e.g., ['dashboards/openshift-cluster-sample-dashboard.yaml', 'dashboards/perses-dashboard-sample.yaml', ...]) and loop over that array calling cy.exec with `${baseFolder}/${manifest}` and Cypress.env('KUBECONFIG_PATH'); apply the same refactor for the cleanup section referenced around lines 111-143 so setup and teardown share the same manifest list.web/cypress/configure-env.sh (2)
15-26: Optional: suppress ShellCheck SC1090 for the dynamic source.The static analysis flagged line 21 (
source "$_cfg_out") because ShellCheck can't follow non-constant sources. A directive silences this known false positive.Proposed fix
if [[ $_cfg_rc -eq 0 && -f "$_cfg_out" && "$*" != *--show* && "$*" != *-s* ]]; then + # shellcheck source=/dev/null source "$_cfg_out"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/configure-env.sh` around lines 15 - 26, Add a ShellCheck directive to suppress SC1090 for the dynamic source call so the linter stops flagging the non-constant include; specifically, above the line that does source "$_cfg_out" (the dynamic sourcing using the _cfg_out variable in the conditional block) insert a comment like "# shellcheck disable=SC1090" to silence the false positive, keeping the rest of the conditional and the _cfg_out/_cfg_dir handling unchanged.
94-110: Dead branch: the""case on line 106 is unreachable.
ynis already defaulted on line 102 (yn=${yn:-$default_answer}), so it will never be empty when thecasestatement is evaluated. The[Yy]and[Nn]branches will always match first.Proposed cleanup
case "$yn" in [Yy]|[Yy][Ee][Ss]) echo "y"; return 0;; [Nn]|[Nn][Oo]) echo "n"; return 0;; - "") [[ "$default_answer" =~ ^[Yy]$ ]] && echo "y" || echo "n"; return 0;; esac🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/configure-env.sh` around lines 94 - 110, The ask_yes_no() function defaults yn on line with "yn=${yn:-$default_answer}", making the case "" branch unreachable; remove the empty-string case from the case statement and let the existing [Yy]|[Yy][Ee][Ss] and [Nn]|[Nn][Oo] branches handle answers (keeping the defaulting behavior intact), or alternatively remove the defaulting assignment and rely on the "" case — choose one approach and update the case in ask_yes_no() accordingly so there are no dead branches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/cypress/configure-env.sh`:
- Around line 337-347: The current pre-fill logic uses ${login_users%%:*} and
${login_users##*:} which split on colons and corrupt usernames/passwords that
contain ':'; update the script to detect when login_users contains exactly one
':' and only then compute prefill_user=${login_users%%:*} and
prefill_pass=${login_users##*:}; if there are zero or multiple colons, pass an
empty default to ask_required (or omit prefill) so we don't truncate values.
Change the two ask_required calls that currently use ${login_users%%:*} and
${login_users##*:} to use these safe prefill variables (computed once from
login_users) or empty when colon-count != 1.
- Around line 482-486: The summary printing loop prints raw entries from the
export_lines array (used in print_current_config) and exposes sensitive values
like CYPRESS_LOGIN_USERS; change the loop that echoes "${line#export }" to
detect exports for CYPRESS_LOGIN_USERS (and any other sensitive keys) and
replace the value with a masked placeholder (e.g., **** or redacted) before
printing, or call the existing masking helper used by print_current_config so
the final "Configured values:" block never emits cleartext passwords.
In `@web/cypress/support/commands/auth-commands.ts`:
- Around line 99-153: The session key builders (generateCOOSessionKey,
generateMPSessionKey, generateKBVSessionKey) currently drop falsy env vars with
envVars.filter(Boolean), causing positional collisions; preserve positions by
mapping each environment value to a sentinel when unset (e.g. '__unset__' or
empty string) instead of filtering so the key retains slot information—replace
the envVars.filter(Boolean) usage with a mapped array that emits the actual env
var when present and a sentinel when falsy for all three functions.
In `@web/cypress/support/commands/coo-install-commands.ts`:
- Around line 32-33: The occurance of the literal command "oc create namespace
${MCP.namespace} --kubeconfig ${Cypress.env('KUBECONFIG_PATH')}" is not
idempotent and will fail if the namespace already exists; replace both
occurrences with an idempotent creation (for example, pipe a Namespace manifest
with metadata.name set to MCP.namespace to oc apply --kubeconfig
${Cypress.env('KUBECONFIG_PATH')} or fallback to a safe create such as "oc
create namespace ... || true") so the setup can be rerun; update the strings
where the command is constructed (the template using MCP.namespace and
Cypress.env('KUBECONFIG_PATH')) accordingly.
- Around line 196-218: The current cy.exec(`oc get ns ... -o jsonpath=...`)
assumes a missing namespace yields stdout 'NotFound', but oc returns a non-zero
exit and message in result.stderr, so the status === 'NotFound' branch is
unreachable; update the check in the cy.exec.then((result) => { ... }) handler
for the oc get call (the block referencing result, MCP.namespace and calling
checkStatus) to detect namespace-deleted by inspecting result.code !== 0 and/or
result.stderr (e.g., contains 'NotFound' or 'not found') instead of only
result.stdout, and treat that case as the successful deletion path while keeping
the existing Terminating and unexpected-state handling.
- Around line 246-253: The pod-deletion check can false-pass because the command
redirects stderr into stdout (`2>&1`) and then counts lines, hiding CLI
failures; modify the cy.exec call (that uses `oc get pods -n ${namespace}
--kubeconfig ${kubeconfigPath} -o name | grep -E '${podPatterns}' | wc -l`) to
stop redirecting stderr, keep failOnNonZeroExit: false, and after the exec
inspect result.code and result.stderr before trusting result.stdout: if
result.code !== 0 or result.stderr contains an error (e.g., auth/network/API
errors or "not found") treat it as a failure, otherwise parse result.stdout to
compute count and only then decide if pods are deleted (compare count === 0 and
log using elapsed); reference the existing cy.exec call, variables
podPatterns/kubeconfigPath/namespace, and the result/count/elapsed checks to
implement this change.
- Around line 123-125: The .should() assertion is passed an unsupported third
argument (a timeout); remove the timeout from the .should() call so the
assertion reads like cy.get('[data-test="status-text"]', { timeout:
installTimeoutMilliseconds }).eq(0).should('contain.text', 'Succeeded') — the
timeout is already applied on cy.get() and will be inherited by the assertion.
In `@web/cypress/support/commands/dashboards-commands.ts`:
- Line 19: The cy.exec call that runs the one-shot OpenShift creation
("cy.exec(... oc new-project perses-dev ... )") is not idempotent and will fail
if the namespace/project already exists; update dashboards-commands.ts to make
that step idempotent by first checking for the presence of the perses-dev
project (using oc get project/namespace) and only invoking oc new-project when
it does not exist, or otherwise switch to an oc project/namespace command to
select the existing project; modify the code path where the cy.exec for creating
the project is invoked so the command sequence is conditional and will not fail
on reruns.
In `@web/cypress/support/commands/operator-commands.ts`:
- Around line 136-142: The cleanupMP Cypress command currently skips reverting
images when useSession is false, leaving stale image state; update the
Cypress.Commands.add('cleanupMP', ...) implementation so that
imagePatchUtils.revertMonitoringPluginImage(MP) is invoked regardless of the
useSession flag (either remove the if (useSession) guard or add an else branch
that calls imagePatchUtils.revertMonitoringPluginImage(MP)), ensuring the revert
runs every time cleanupMP is called.
- Line 52: The SESSION env is read as a string so 'false' is truthy; update the
useSession initialization (const useSession) to coerce the env to a strict
boolean by reading Cypress.env('SESSION'), converting to string, lowercasing,
and comparing to 'true' (defaulting to false when unset) so downstream checks
like if (useSession) behave correctly.
---
Outside diff comments:
In `@web/cypress/e2e/incidents/01.incidents.cy.ts`:
- Line 98: The test is referencing incidentsPage.clearAllFilters as a property
instead of invoking it, so filters aren't being cleared; change the reference to
a function call by invoking incidentsPage.clearAllFilters() wherever it's used
(e.g., the occurrence in the test at the given diff) so the method executes and
clears filters before step 5.3.
---
Nitpick comments:
In `@web/cypress/configure-env.sh`:
- Around line 15-26: Add a ShellCheck directive to suppress SC1090 for the
dynamic source call so the linter stops flagging the non-constant include;
specifically, above the line that does source "$_cfg_out" (the dynamic sourcing
using the _cfg_out variable in the conditional block) insert a comment like "#
shellcheck disable=SC1090" to silence the false positive, keeping the rest of
the conditional and the _cfg_out/_cfg_dir handling unchanged.
- Around line 94-110: The ask_yes_no() function defaults yn on line with
"yn=${yn:-$default_answer}", making the case "" branch unreachable; remove the
empty-string case from the case statement and let the existing [Yy]|[Yy][Ee][Ss]
and [Nn]|[Nn][Oo] branches handle answers (keeping the defaulting behavior
intact), or alternatively remove the defaulting assignment and rely on the ""
case — choose one approach and update the case in ask_yes_no() accordingly so
there are no dead branches.
In `@web/cypress/support/commands/dashboards-commands.ts`:
- Around line 24-58: The two branches gated by Cypress.env('COO_UI_INSTALL')
duplicate the same list of cy.exec manifest applies (the multiple cy.exec lines
in the block guarded by COO_UI_INSTALL and its else), causing drift; replace the
duplicated blocks by computing a baseFolder variable (e.g., coo121_perses vs
coo141_perses) based on Cypress.env('COO_UI_INSTALL'), store the manifest
filenames in a single array (e.g.,
['dashboards/openshift-cluster-sample-dashboard.yaml',
'dashboards/perses-dashboard-sample.yaml', ...]) and loop over that array
calling cy.exec with `${baseFolder}/${manifest}` and
Cypress.env('KUBECONFIG_PATH'); apply the same refactor for the cleanup section
referenced around lines 111-143 so setup and teardown share the same manifest
list.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (17)
web/cypress/configure-env.shweb/cypress/e2e/incidents/00.coo_incidents_e2e.cy.tsweb/cypress/e2e/incidents/01.incidents.cy.tsweb/cypress/e2e/incidents/02.incidents-mocking-example.cy.tsweb/cypress/e2e/incidents/regression/01.reg_filtering.cy.tsweb/cypress/e2e/incidents/regression/02.reg_ui_charts_comprehensive.cy.tsweb/cypress/e2e/incidents/regression/03-04.reg_e2e_firing_alerts.cy.tsweb/cypress/e2e/incidents/regression/03.reg_api_calls.cy.tsweb/cypress/e2e/incidents/regression/04.reg_redux_effects.cy.tsweb/cypress/support/commands/auth-commands.tsweb/cypress/support/commands/coo-install-commands.tsweb/cypress/support/commands/dashboards-commands.tsweb/cypress/support/commands/image-patch-commands.tsweb/cypress/support/commands/operator-commands.tsweb/cypress/support/commands/virtualization-commands.tsweb/cypress/support/commands/wait-utils.tsweb/cypress/support/index.ts
Comment on lines
+337
to
347
| local login_user login_pass | ||
| if [[ "$login_choice" == "1" ]]; then | ||
| login_idp="kube:admin" | ||
| else | ||
| login_idp=$(ask_required "Login identity provider name" "${def_login_idp:-flexy-htpasswd-provider}") | ||
| fi | ||
|
|
||
| local login_user login_pass login_users | ||
| if [[ "$login_idp" == "kube:admin" ]]; then | ||
| login_user="kubeadmin" | ||
| login_pass=$(ask_required "kubeadmin password" "${def_login_users##*:}") | ||
| login_pass=$(ask_required "kubeadmin password" "${login_users##*:}") | ||
| else | ||
| login_user=$(ask_required "Username" "${def_login_users%%:*}") | ||
| login_pass=$(ask_required "Password" "${def_login_users##*:}") | ||
| login_idp=$(ask_required "Identity provider name" "${login_idp:-flexy-htpasswd-provider}") | ||
| login_user=$(ask_required "Username" "${login_users%%:*}") | ||
| login_pass=$(ask_required "Password" "${login_users##*:}") | ||
| fi | ||
| login_users="$login_user:$login_pass" |
There was a problem hiding this comment.
Passwords containing : will be truncated in the default pre-fill.
${login_users##*:} (line 341, 345) grabs only the portion after the last colon, so a password like p@ss:word would pre-fill as just word. Similarly, a username containing : would break the %%:* extraction on line 344.
This is inherent to the user:pass format and likely pre-existing, but worth noting — especially if CI service-account passwords may contain colons.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/cypress/configure-env.sh` around lines 337 - 347, The current pre-fill
logic uses ${login_users%%:*} and ${login_users##*:} which split on colons and
corrupt usernames/passwords that contain ':'; update the script to detect when
login_users contains exactly one ':' and only then compute
prefill_user=${login_users%%:*} and prefill_pass=${login_users##*:}; if there
are zero or multiple colons, pass an empty default to ask_required (or omit
prefill) so we don't truncate values. Change the two ask_required calls that
currently use ${login_users%%:*} and ${login_users##*:} to use these safe
prefill variables (computed once from login_users) or empty when colon-count !=
1.
|
|
||
| setupDashboardsAndPlugins(MCP: { namespace: string }): void { | ||
| cy.log('Create perses-dev namespace.'); | ||
| cy.exec(`oc new-project perses-dev --kubeconfig ${Cypress.env('KUBECONFIG_PATH')}`); |
There was a problem hiding this comment.
Avoid failing on existing perses-dev namespace.
On Line 19, oc new-project perses-dev is not idempotent and can fail reruns when the namespace already exists.
♻️ Suggested fix
- cy.exec(`oc new-project perses-dev --kubeconfig ${Cypress.env('KUBECONFIG_PATH')}`);
+ cy.exec(`oc get namespace perses-dev --kubeconfig ${Cypress.env('KUBECONFIG_PATH')}`, {
+ failOnNonZeroExit: false,
+ }).then((res) => {
+ if (res.code !== 0) {
+ cy.exec(`oc new-project perses-dev --kubeconfig ${Cypress.env('KUBECONFIG_PATH')}`);
+ }
+ });📝 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.
Suggested change
| cy.exec(`oc new-project perses-dev --kubeconfig ${Cypress.env('KUBECONFIG_PATH')}`); | |
| cy.exec(`oc get namespace perses-dev --kubeconfig ${Cypress.env('KUBECONFIG_PATH')}`, { | |
| failOnNonZeroExit: false, | |
| }).then((res) => { | |
| if (res.code !== 0) { | |
| cy.exec(`oc new-project perses-dev --kubeconfig ${Cypress.env('KUBECONFIG_PATH')}`); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/cypress/support/commands/dashboards-commands.ts` at line 19, The cy.exec
call that runs the one-shot OpenShift creation ("cy.exec(... oc new-project
perses-dev ... )") is not idempotent and will fail if the namespace/project
already exists; update dashboards-commands.ts to make that step idempotent by
first checking for the presence of the perses-dev project (using oc get
project/namespace) and only invoking oc new-project when it does not exist, or
otherwise switch to an oc project/namespace command to select the existing
project; modify the code path where the cy.exec for creating the project is
invoked so the command sequence is conditional and will not fail on reruns.
Comment on lines
+136
to
+142
| Cypress.Commands.add('cleanupMP', (MP: { namespace: string; operatorName: string }) => { | ||
| if (useSession) { | ||
| cy.log('cleanupMP (session)'); | ||
| imagePatchUtils.revertMonitoringPluginImage(MP); | ||
| cy.log('cleanupMP (session) completed'); | ||
| } | ||
| }); |
There was a problem hiding this comment.
cleanupMP currently no-ops when sessions are disabled.
In non-session mode, beforeBlock still calls cy.cleanupMP(MP), but this command does nothing due to the if (useSession) guard. That leaves stale image state between runs.
🧹 Suggested fix
Cypress.Commands.add('cleanupMP', (MP: { namespace: string; operatorName: string }) => {
- if (useSession) {
- cy.log('cleanupMP (session)');
- imagePatchUtils.revertMonitoringPluginImage(MP);
- cy.log('cleanupMP (session) completed');
- }
+ cy.log(useSession ? 'cleanupMP (session)' : 'cleanupMP (no session)');
+ imagePatchUtils.revertMonitoringPluginImage(MP);
+ cy.log('cleanupMP completed');
});📝 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.
Suggested change
| Cypress.Commands.add('cleanupMP', (MP: { namespace: string; operatorName: string }) => { | |
| if (useSession) { | |
| cy.log('cleanupMP (session)'); | |
| imagePatchUtils.revertMonitoringPluginImage(MP); | |
| cy.log('cleanupMP (session) completed'); | |
| } | |
| }); | |
| Cypress.Commands.add('cleanupMP', (MP: { namespace: string; operatorName: string }) => { | |
| cy.log(useSession ? 'cleanupMP (session)' : 'cleanupMP (no session)'); | |
| imagePatchUtils.revertMonitoringPluginImage(MP); | |
| cy.log('cleanupMP completed'); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/cypress/support/commands/operator-commands.ts` around lines 136 - 142,
The cleanupMP Cypress command currently skips reverting images when useSession
is false, leaving stale image state; update the
Cypress.Commands.add('cleanupMP', ...) implementation so that
imagePatchUtils.revertMonitoringPluginImage(MP) is invoked regardless of the
useSession flag (either remove the if (useSession) guard or add an else branch
that calls imagePatchUtils.revertMonitoringPluginImage(MP)), ensuring the revert
runs every time cleanupMP is called.
Contributor
Author
|
/test e2e-coo |
Contributor
Author
|
/test all |
Contributor
Author
|
/test e2e-inicidents |
Contributor
Author
|
/test e2e-monitoring |
After page reload, dynamic plugins (like Troubleshooting Panel) may not be registered immediately in the OpenShift Console. Replace the single click-and-assert with a waitUntil loop that closes and re-opens the Application Launcher dropdown until the "Signal Correlation" item appears, up to 60 seconds. Made-with: Cursor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
web/cypress/support/commands/dashboards-commands.ts (1)
20-20:⚠️ Potential issue | 🟠 MajorMake
perses-devproject creation idempotent.Line 20 uses one-shot
oc new-project, which fails on reruns when the namespace already exists.♻️ Suggested fix
- cy.exec(`oc new-project perses-dev --kubeconfig ${Cypress.env('KUBECONFIG_PATH')}`); + cy.exec(`oc get namespace perses-dev --kubeconfig ${Cypress.env('KUBECONFIG_PATH')}`, { + failOnNonZeroExit: false, + }).then((res) => { + if (res.code !== 0) { + cy.exec(`oc new-project perses-dev --kubeconfig ${Cypress.env('KUBECONFIG_PATH')}`); + } + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/support/commands/dashboards-commands.ts` at line 20, The current cy.exec call uses a one-shot "oc new-project perses-dev" which fails if the project already exists; replace that single command with an idempotent shell check such as running "oc get project perses-dev --kubeconfig ${Cypress.env('KUBECONFIG_PATH')}" and only invoking "oc new-project perses-dev --kubeconfig ${Cypress.env('KUBECONFIG_PATH')}" if the get fails (e.g. use "oc get project perses-dev --kubeconfig ... || oc new-project perses-dev --kubeconfig ..."); update the cy.exec invocation in dashboards-commands.ts that contains the string "oc new-project perses-dev --kubeconfig ${Cypress.env('KUBECONFIG_PATH')}" to use this conditional form so repeated test runs do not error.
🧹 Nitpick comments (1)
web/cypress/support/commands/dashboards-commands.ts (1)
25-59: Reduce duplicated dashboard apply/delete command blocks.The COO 1.2.1 vs 1.4.1 branches duplicate the same file list twice (setup and cleanup), which is easy to drift.
♻️ Suggested refactor
+const dashboardFiles = [ + 'openshift-cluster-sample-dashboard.yaml', + 'perses-dashboard-sample.yaml', + 'prometheus-overview-variables.yaml', + 'thanos-compact-overview-1var.yaml', + 'thanos-querier-datasource.yaml', +]; + +const persesFolder = Cypress.env('COO_UI_INSTALL') ? 'coo121_perses' : 'coo141_perses'; + // setup - if (Cypress.env('COO_UI_INSTALL')) { - ... - } else { - ... - } + dashboardFiles.forEach((file) => { + cy.exec( + `oc apply -f ./cypress/fixtures/coo/${persesFolder}/dashboards/${file} --kubeconfig ${Cypress.env('KUBECONFIG_PATH')}`, + ); + }); // cleanup - if (Cypress.env('COO_UI_INSTALL')) { - ... - } else { - ... - } + dashboardFiles.forEach((file) => { + cy.executeAndDelete( + `oc delete -f ./cypress/fixtures/coo/${persesFolder}/dashboards/${file} --ignore-not-found --kubeconfig ${Cypress.env('KUBECONFIG_PATH')}`, + ); + });Also applies to: 134-166
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/support/commands/dashboards-commands.ts` around lines 25 - 59, The duplicated cy.exec blocks for applying dashboard manifests should be collapsed by creating a single array of relative dashboard paths and a computed base folder based on Cypress.env('COO_UI_INSTALL'), then iterate that array and call cy.exec for each file using Cypress.env('KUBECONFIG_PATH'); update the same approach for the corresponding cleanup (delete) block mentioned (lines 134-166) so both setup and teardown share the same file list and looping logic; look for the cy.exec calls and the Cypress.env('COO_UI_INSTALL') check to replace with a single function or loop that builds the full path (e.g., `${baseFolder}/${file}`) and executes oc apply/delete for each entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@web/cypress/support/commands/dashboards-commands.ts`:
- Line 20: The current cy.exec call uses a one-shot "oc new-project perses-dev"
which fails if the project already exists; replace that single command with an
idempotent shell check such as running "oc get project perses-dev --kubeconfig
${Cypress.env('KUBECONFIG_PATH')}" and only invoking "oc new-project perses-dev
--kubeconfig ${Cypress.env('KUBECONFIG_PATH')}" if the get fails (e.g. use "oc
get project perses-dev --kubeconfig ... || oc new-project perses-dev
--kubeconfig ..."); update the cy.exec invocation in dashboards-commands.ts that
contains the string "oc new-project perses-dev --kubeconfig
${Cypress.env('KUBECONFIG_PATH')}" to use this conditional form so repeated test
runs do not error.
---
Nitpick comments:
In `@web/cypress/support/commands/dashboards-commands.ts`:
- Around line 25-59: The duplicated cy.exec blocks for applying dashboard
manifests should be collapsed by creating a single array of relative dashboard
paths and a computed base folder based on Cypress.env('COO_UI_INSTALL'), then
iterate that array and call cy.exec for each file using
Cypress.env('KUBECONFIG_PATH'); update the same approach for the corresponding
cleanup (delete) block mentioned (lines 134-166) so both setup and teardown
share the same file list and looping logic; look for the cy.exec calls and the
Cypress.env('COO_UI_INSTALL') check to replace with a single function or loop
that builds the full path (e.g., `${baseFolder}/${file}`) and executes oc
apply/delete for each entry.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
web/cypress/support/commands/dashboards-commands.ts
- Normalize SESSION env var to strict boolean (string 'false' was truthy) - Preserve positional info in session keys (filter(Boolean) -> map) - Make namespace creation idempotent with --dry-run=client | oc apply - Make oc new-project perses-dev tolerant of existing namespace - Remove unsupported timeout arg from .should() assertion - Fix unreachable NotFound branch in namespace deletion polling - Fix pod-deletion check false-passing on CLI failures Made-with: Cursor
Contributor
Author
|
/test e2e-monitoring |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
web/cypress/support/commands/operator-commands.ts (1)
136-142:⚠️ Potential issue | 🟠 Major
cleanupMPshould not be a no-op when session mode is off.When
useSessionis false, cleanup is skipped entirely, leaving stale patched image state between runs.♻️ Suggested fix
Cypress.Commands.add('cleanupMP', (MP: { namespace: string; operatorName: string }) => { - if (useSession) { - cy.log('cleanupMP (session)'); - imagePatchUtils.revertMonitoringPluginImage(MP); - cy.log('cleanupMP (session) completed'); - } + cy.log(useSession ? 'cleanupMP (session)' : 'cleanupMP (no session)'); + imagePatchUtils.revertMonitoringPluginImage(MP); + cy.log('cleanupMP completed'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/support/commands/operator-commands.ts` around lines 136 - 142, The cleanupMP command currently only reverts the monitoring plugin image when useSession is true, causing stale state when useSession is false; modify Cypress.Commands.add('cleanupMP', ...) so it always calls imagePatchUtils.revertMonitoringPluginImage(MP) (performing the revert outside or in both branches of the useSession check) and keep or adjust the cy.log messages as needed so cleanup runs regardless of the useSession flag.web/cypress/support/commands/coo-install-commands.ts (1)
200-203:⚠️ Potential issue | 🟠 MajorDon’t treat every
oc get nsfailure as “namespace deleted.”Any non-zero exit currently counts as successful deletion. That can hide auth/network/API failures and stop cleanup polling too early.
♻️ Suggested fix
- if (result.code !== 0) { - cy.log(`${elapsed}ms - ${MCP.namespace} is successfully deleted.`); - return; - } + if (result.code !== 0) { + const err = (result.stderr || '').toLowerCase(); + if (err.includes('notfound') || err.includes('not found')) { + cy.log(`${elapsed}ms - ${MCP.namespace} is successfully deleted.`); + return; + } + cy.log(`${elapsed}ms - Failed to query namespace status: ${result.stderr}. Retrying...`); + cy.wait(checkIntervalMs).then(checkStatus); + return; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/support/commands/coo-install-commands.ts` around lines 200 - 203, The current check treats any non-zero result.code from the oc get namespace call as successful deletion; instead change the logic in the function handling this result (look for uses of result.code, elapsed, and MCP.namespace in coo-install-commands.ts) so that you only consider the namespace deleted when the command output explicitly indicates the namespace is missing (e.g., result.stdout or result.stderr contains "NotFound" or "Error from server (NotFound)"); for other non-zero exit codes log the full error (include stdout/stderr), do not return early, and allow the polling/retry to continue or surface an error instead of treating it as success.web/cypress/support/commands/dashboards-commands.ts (1)
20-20:⚠️ Potential issue | 🟠 MajorHandle
new-projecterrors explicitly instead of ignoring all non-zero exits.This currently swallows every failure, not just “already exists”, which can mask real cluster/access problems.
♻️ Suggested fix
- cy.exec(`oc new-project perses-dev --kubeconfig ${Cypress.env('KUBECONFIG_PATH')}`, { failOnNonZeroExit: false }); + cy.exec(`oc new-project perses-dev --kubeconfig ${Cypress.env('KUBECONFIG_PATH')}`, { + failOnNonZeroExit: false, + }).then((result) => { + const err = (result.stderr || '').toLowerCase(); + if (result.code !== 0 && !err.includes('already exists')) { + throw new Error(`Failed to create/select perses-dev project: ${result.stderr || result.stdout}`); + } + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/support/commands/dashboards-commands.ts` at line 20, The cy.exec call that creates the OpenShift project (cy.exec(`oc new-project perses-dev --kubeconfig ${Cypress.env('KUBECONFIG_PATH')}`)) currently ignores all non-zero exits; change it to capture the result (use failOnNonZeroExit: false), then inspect the returned result.code and result.stdout/result.stderr for the benign "already exists" message (e.g., "already exists" / "AlreadyExists"); if the exit code is non-zero and the output does NOT indicate the project already exists, throw/propagate an error including the original stdout/stderr so real failures surface instead of being swallowed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/cypress/support/commands/auth-commands.ts`:
- Around line 57-67: Replace the success check that currently uses result.stderr
=== '' with a proper exit-code check: test that result.code === 0 (success),
then parse and trim result.stdout before building the URL (use const oauth =
result.stdout.trim()), construct the URL (oauthurl) and derive oauthorigin as
before; otherwise throw the Error including result.code, result.stdout and
result.stderr as currently done. Ensure you update the block that assigns oauth,
oauthurl and oauthorigin to use the trimmed stdout and the success condition on
result.code.
---
Duplicate comments:
In `@web/cypress/support/commands/coo-install-commands.ts`:
- Around line 200-203: The current check treats any non-zero result.code from
the oc get namespace call as successful deletion; instead change the logic in
the function handling this result (look for uses of result.code, elapsed, and
MCP.namespace in coo-install-commands.ts) so that you only consider the
namespace deleted when the command output explicitly indicates the namespace is
missing (e.g., result.stdout or result.stderr contains "NotFound" or "Error from
server (NotFound)"); for other non-zero exit codes log the full error (include
stdout/stderr), do not return early, and allow the polling/retry to continue or
surface an error instead of treating it as success.
In `@web/cypress/support/commands/dashboards-commands.ts`:
- Line 20: The cy.exec call that creates the OpenShift project (cy.exec(`oc
new-project perses-dev --kubeconfig ${Cypress.env('KUBECONFIG_PATH')}`))
currently ignores all non-zero exits; change it to capture the result (use
failOnNonZeroExit: false), then inspect the returned result.code and
result.stdout/result.stderr for the benign "already exists" message (e.g.,
"already exists" / "AlreadyExists"); if the exit code is non-zero and the output
does NOT indicate the project already exists, throw/propagate an error including
the original stdout/stderr so real failures surface instead of being swallowed.
In `@web/cypress/support/commands/operator-commands.ts`:
- Around line 136-142: The cleanupMP command currently only reverts the
monitoring plugin image when useSession is true, causing stale state when
useSession is false; modify Cypress.Commands.add('cleanupMP', ...) so it always
calls imagePatchUtils.revertMonitoringPluginImage(MP) (performing the revert
outside or in both branches of the useSession check) and keep or adjust the
cy.log messages as needed so cleanup runs regardless of the useSession flag.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
web/cypress/support/commands/auth-commands.tsweb/cypress/support/commands/coo-install-commands.tsweb/cypress/support/commands/dashboards-commands.tsweb/cypress/support/commands/operator-commands.ts
Comment on lines
+57
to
+67
| if (result.stderr === '') { | ||
| const oauth = result.stdout; | ||
| const oauthurl = new URL(oauth); | ||
| const oauthorigin = oauthurl.origin; | ||
| cy.log(oauthorigin); | ||
| cy.wrap(oauthorigin as string).as('oauthorigin'); | ||
| } else { | ||
| throw new Error(`Execution of oc get oauthclient failed | ||
| Exit code: ${result.code} | ||
| Stdout:\n${result.stdout} | ||
| Stderr:\n${result.stderr}`); |
There was a problem hiding this comment.
Use exit code to detect oc success, not empty stderr.
Relying on stderr === '' can false-fail valid executions (warnings). Use result.code === 0 and parse trimmed stdout.
♻️ Suggested fix
- if (result.stderr === '') {
- const oauth = result.stdout;
+ if (result.code === 0) {
+ const oauth = result.stdout.trim();
const oauthurl = new URL(oauth);
const oauthorigin = oauthurl.origin;
cy.log(oauthorigin);
cy.wrap(oauthorigin as string).as('oauthorigin');
} else {
throw new Error(`Execution of oc get oauthclient failed
Exit code: ${result.code}
Stdout:\n${result.stdout}
Stderr:\n${result.stderr}`);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/cypress/support/commands/auth-commands.ts` around lines 57 - 67, Replace
the success check that currently uses result.stderr === '' with a proper
exit-code check: test that result.code === 0 (success), then parse and trim
result.stdout before building the URL (use const oauth = result.stdout.trim()),
construct the URL (oauthurl) and derive oauthorigin as before; otherwise throw
the Error including result.code, result.stdout and result.stderr as currently
done. Ensure you update the block that assigns oauth, oauthurl and oauthorigin
to use the trimmed stdout and the success condition on result.code.
Contributor
|
/test e2e-monitoring-dev |
Contributor
|
@DavidRajnoha: 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. |
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
4 participants
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
configure-env.shwith logical prompt ordering (Cluster → Install Mode → Operators → Runtime), gating irrelevant questions based on earlier answers, fixingset -euo pipefailcompatibility, and adding zsh supportoperator-commands.ts(932 lines) into focused modules (auth-commands.ts,coo-install-commands.ts,image-patch-commands.ts,dashboards-commands.ts) with a slim orchestrator, and addCOOSetupOptionsso callers (e.g. incident tests) can skip unneeded Perses/korrel8r/CHA setupsleepcalls in operator setup withcy.waitUntil-based polling via sharedwait-utils.tshelpers (waitForPodsReady,waitForPodsReadyOrAbsent,waitForResourceCondition)Summary by CodeRabbit
Tests
Chores