NO-JIRA: perses-dev with 6th user#80804
Conversation
|
@etmurasaki: 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. |
|
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:
WalkthroughThe PR introduces a new Perses UI IVT (In-Vitro Testing) test with complete step-registry definition, test script, and pipeline job integration for both standard and periodic schedules. It increments the perses-dev-ui user generation loop from 5 to 6 users, removes HTPasswd identity provider setup from the perses-ui test script, and corrects step documentation strings to accurately describe Perses UI Cypress tests. ChangesPerses UI Testing Infrastructure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
ce2db99 to
f0f7c20
Compare
|
[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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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
`@ci-operator/step-registry/monitoring-plugin/tests/perses-ui-ivt/monitoring-plugin-tests-perses-ui-ivt-commands.sh`:
- Around line 37-45: The script currently exits with status 0 (success) when
encountering errors in the KUBEADMIN_PASSWORD_FILE prerequisite checks, which
masks failures and allows the test to be skipped while appearing successful.
Change both exit 0 statements (one in the variable existence check and one in
the file existence check) to exit 1 to properly signal failure when these
critical prerequisites are missing.
- Around line 104-114: The while loop controlled by the wait_auth variable has
no timeout mechanism and can hang indefinitely if the OAuth deployment rollout
never converges. Add a timeout to this polling loop by either tracking elapsed
time with a start timestamp and breaking if the elapsed time exceeds a defined
maximum (such as 300 seconds), or by maintaining an iteration counter and
breaking after a maximum number of retries. Ensure the timeout is checked within
the loop condition or at the beginning of each iteration to prevent unbounded
waiting.
In
`@ci-operator/step-registry/monitoring-plugin/tests/perses-ui-ivt/monitoring-plugin-tests-perses-ui-ivt-ref.yaml`:
- Around line 64-65: The documentation field at lines 64-65 contains misleading
text that references "monitoring-plugin-tests-monitoring-ui" when the actual
step is "monitoring-plugin-tests-perses-ui-ivt". Update the documentation text
to replace the reference to "monitoring-ui" with "perses-ui-ivt" so that the
description accurately matches the step name and what it actually does.
🪄 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: 058d41cc-3745-4b2c-bc89-43517aefc58c
⛔ Files ignored due to path filters (2)
ci-operator/jobs/openshift/monitoring-plugin/openshift-monitoring-plugin-main-periodics.yamlis excluded by!ci-operator/jobs/**ci-operator/jobs/openshift/monitoring-plugin/openshift-monitoring-plugin-main-presubmits.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (7)
ci-operator/config/openshift/monitoring-plugin/openshift-monitoring-plugin-main.yamlci-operator/config/openshift/monitoring-plugin/openshift-monitoring-plugin-main__periodics.yamlci-operator/step-registry/monitoring-plugin/tests/perses-dev-ui/monitoring-plugin-tests-perses-dev-ui-commands.shci-operator/step-registry/monitoring-plugin/tests/perses-ui-ivt/OWNERSci-operator/step-registry/monitoring-plugin/tests/perses-ui-ivt/monitoring-plugin-tests-perses-ui-ivt-commands.shci-operator/step-registry/monitoring-plugin/tests/perses-ui-ivt/monitoring-plugin-tests-perses-ui-ivt-ref.metadata.jsonci-operator/step-registry/monitoring-plugin/tests/perses-ui-ivt/monitoring-plugin-tests-perses-ui-ivt-ref.yaml
✅ Files skipped from review due to trivial changes (2)
- ci-operator/step-registry/monitoring-plugin/tests/perses-ui-ivt/monitoring-plugin-tests-perses-ui-ivt-ref.metadata.json
- ci-operator/step-registry/monitoring-plugin/tests/perses-ui-ivt/OWNERS
🚧 Files skipped from review as they are similar to previous changes (1)
- ci-operator/step-registry/monitoring-plugin/tests/perses-dev-ui/monitoring-plugin-tests-perses-dev-ui-commands.sh
f0f7c20 to
dbdb674
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
ci-operator/step-registry/monitoring-plugin/tests/perses-ui-ivt/monitoring-plugin-tests-perses-ui-ivt-commands.sh (2)
27-34: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider defensive handling for indirect expansion with
nounset.With
set -o nounset, if any variable invarsis completely unset (not merely empty), the${!var}indirect expansion will fail before the-ztest runs. If ci-operator guarantees these are always set (even to empty strings) via the ref.yaml env definitions, this is fine. Otherwise, using${!var:-}provides safer handling.for var in "${vars[@]}"; do - if [[ -z "${!var}" ]]; then + if [[ -z "${!var:-}" ]]; then unset "$var"🤖 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 `@ci-operator/step-registry/monitoring-plugin/tests/perses-ui-ivt/monitoring-plugin-tests-perses-ui-ivt-commands.sh` around lines 27 - 34, The indirect variable expansion ${!var} used in the loop will fail with set -o nounset if any variable in the vars array is completely unset, before the -z test can evaluate it. Replace the unsafe indirect expansions ${!var} with the safer pattern ${!var:-} in both the -z check and the echo statement within the loop, which provides a default empty value if the variable doesn't exist, ensuring compatibility with nounset and preventing script failure on unset variables.
137-142: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winRemove debug
ls -ltrcommand.Line 138 lists directory contents but the output isn't used for anything. This appears to be a leftover debug command that adds noise to CI logs.
# Install npm modules -ls -ltr npm install🤖 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 `@ci-operator/step-registry/monitoring-plugin/tests/perses-ui-ivt/monitoring-plugin-tests-perses-ui-ivt-commands.sh` around lines 137 - 142, Remove the standalone `ls -ltr` command that appears after the "# Install npm modules" comment. This debug command serves no functional purpose in the script and only adds unnecessary noise to the CI logs. Simply delete the line containing `ls -ltr` so that the npm install command directly follows the install modules comment.
🤖 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
`@ci-operator/step-registry/monitoring-plugin/tests/perses-ui-ivt/monitoring-plugin-tests-perses-ui-ivt-commands.sh`:
- Line 56: The KUBECONFIG variable in the oc get clusteroperator command is
unquoted, which can cause word splitting issues if the path contains spaces.
Wrap the variable reference ${KUBECONFIG} with double quotes so it becomes
"${KUBECONFIG}" to ensure the entire path is treated as a single argument in the
oc command invocation.
- Line 121: In the cp command on line 121, the $KUBECONFIG variable is unquoted
which can cause word splitting if the path contains spaces or special
characters. Quote the $KUBECONFIG variable by changing $KUBECONFIG to
"$KUBECONFIG" in the cp -L command to prevent word splitting issues.
---
Nitpick comments:
In
`@ci-operator/step-registry/monitoring-plugin/tests/perses-ui-ivt/monitoring-plugin-tests-perses-ui-ivt-commands.sh`:
- Around line 27-34: The indirect variable expansion ${!var} used in the loop
will fail with set -o nounset if any variable in the vars array is completely
unset, before the -z test can evaluate it. Replace the unsafe indirect
expansions ${!var} with the safer pattern ${!var:-} in both the -z check and the
echo statement within the loop, which provides a default empty value if the
variable doesn't exist, ensuring compatibility with nounset and preventing
script failure on unset variables.
- Around line 137-142: Remove the standalone `ls -ltr` command that appears
after the "# Install npm modules" comment. This debug command serves no
functional purpose in the script and only adds unnecessary noise to the CI logs.
Simply delete the line containing `ls -ltr` so that the npm install command
directly follows the install modules comment.
🪄 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: c5046786-807f-4f6a-8c81-c83b09aa8583
⛔ Files ignored due to path filters (2)
ci-operator/jobs/openshift/monitoring-plugin/openshift-monitoring-plugin-main-periodics.yamlis excluded by!ci-operator/jobs/**ci-operator/jobs/openshift/monitoring-plugin/openshift-monitoring-plugin-main-presubmits.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (9)
ci-operator/config/openshift/monitoring-plugin/openshift-monitoring-plugin-main.yamlci-operator/config/openshift/monitoring-plugin/openshift-monitoring-plugin-main__periodics.yamlci-operator/step-registry/monitoring-plugin/tests/perses-dev-ui/monitoring-plugin-tests-perses-dev-ui-commands.shci-operator/step-registry/monitoring-plugin/tests/perses-dev-ui/monitoring-plugin-tests-perses-dev-ui-ref.yamlci-operator/step-registry/monitoring-plugin/tests/perses-ui-ivt/OWNERSci-operator/step-registry/monitoring-plugin/tests/perses-ui-ivt/monitoring-plugin-tests-perses-ui-ivt-commands.shci-operator/step-registry/monitoring-plugin/tests/perses-ui-ivt/monitoring-plugin-tests-perses-ui-ivt-ref.metadata.jsonci-operator/step-registry/monitoring-plugin/tests/perses-ui-ivt/monitoring-plugin-tests-perses-ui-ivt-ref.yamlci-operator/step-registry/monitoring-plugin/tests/perses-ui/monitoring-plugin-tests-perses-ui-ref.yaml
✅ Files skipped from review due to trivial changes (3)
- ci-operator/step-registry/monitoring-plugin/tests/perses-dev-ui/monitoring-plugin-tests-perses-dev-ui-ref.yaml
- ci-operator/step-registry/monitoring-plugin/tests/perses-ui/monitoring-plugin-tests-perses-ui-ref.yaml
- ci-operator/step-registry/monitoring-plugin/tests/perses-ui-ivt/monitoring-plugin-tests-perses-ui-ivt-ref.metadata.json
🚧 Files skipped from review as they are similar to previous changes (5)
- ci-operator/step-registry/monitoring-plugin/tests/perses-dev-ui/monitoring-plugin-tests-perses-dev-ui-commands.sh
- ci-operator/step-registry/monitoring-plugin/tests/perses-ui-ivt/OWNERS
- ci-operator/config/openshift/monitoring-plugin/openshift-monitoring-plugin-main__periodics.yaml
- ci-operator/config/openshift/monitoring-plugin/openshift-monitoring-plugin-main.yaml
- ci-operator/step-registry/monitoring-plugin/tests/perses-ui-ivt/monitoring-plugin-tests-perses-ui-ivt-ref.yaml
dbdb674 to
79aac8f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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
`@ci-operator/step-registry/monitoring-plugin/tests/perses-ui-ivt/monitoring-plugin-tests-perses-ui-ivt-commands.sh`:
- Around line 99-101: The OAuth test IDP setup needs to be made idempotent and
non-destructive. First, replace the `oc create secret` command with `oc apply`
to make secret creation idempotent so it succeeds on reruns without failing if
the secret already exists. Second, the OAuth patch that adds the HTPasswd
identity provider to `/spec/identityProviders` is destructive because it
replaces the entire array, potentially removing existing providers. Instead,
check if the HTPasswd provider named "uiauto-htpasswd-idp" already exists, and
use a JSON patch operation like "add" with an index or "test" operation to
verify the array exists before appending, ensuring existing identity providers
are preserved and the new test IDP is only added once.
- Around line 27-32: The indirect expansion ${!var} in the for loop will cause
the script to abort when the nounset option is enabled and any target variable
is unset, preventing the cleanup logic from executing properly. Modify the
indirect expansion to use a default value syntax such as ${!var:-} or ${!var-}
to gracefully handle cases where the target variable is unset, allowing the loop
to continue and properly unset the variables instead of aborting.
🪄 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: 5c735e85-0110-458c-9637-2c223e27fc5f
⛔ Files ignored due to path filters (2)
ci-operator/jobs/openshift/monitoring-plugin/openshift-monitoring-plugin-main-periodics.yamlis excluded by!ci-operator/jobs/**ci-operator/jobs/openshift/monitoring-plugin/openshift-monitoring-plugin-main-presubmits.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (9)
ci-operator/config/openshift/monitoring-plugin/openshift-monitoring-plugin-main.yamlci-operator/config/openshift/monitoring-plugin/openshift-monitoring-plugin-main__periodics.yamlci-operator/step-registry/monitoring-plugin/tests/perses-dev-ui/monitoring-plugin-tests-perses-dev-ui-commands.shci-operator/step-registry/monitoring-plugin/tests/perses-dev-ui/monitoring-plugin-tests-perses-dev-ui-ref.yamlci-operator/step-registry/monitoring-plugin/tests/perses-ui-ivt/OWNERSci-operator/step-registry/monitoring-plugin/tests/perses-ui-ivt/monitoring-plugin-tests-perses-ui-ivt-commands.shci-operator/step-registry/monitoring-plugin/tests/perses-ui-ivt/monitoring-plugin-tests-perses-ui-ivt-ref.metadata.jsonci-operator/step-registry/monitoring-plugin/tests/perses-ui-ivt/monitoring-plugin-tests-perses-ui-ivt-ref.yamlci-operator/step-registry/monitoring-plugin/tests/perses-ui/monitoring-plugin-tests-perses-ui-ref.yaml
✅ Files skipped from review due to trivial changes (4)
- ci-operator/step-registry/monitoring-plugin/tests/perses-dev-ui/monitoring-plugin-tests-perses-dev-ui-ref.yaml
- ci-operator/step-registry/monitoring-plugin/tests/perses-ui/monitoring-plugin-tests-perses-ui-ref.yaml
- ci-operator/step-registry/monitoring-plugin/tests/perses-ui-ivt/OWNERS
- ci-operator/step-registry/monitoring-plugin/tests/perses-ui-ivt/monitoring-plugin-tests-perses-ui-ivt-ref.metadata.json
🚧 Files skipped from review as they are similar to previous changes (3)
- ci-operator/step-registry/monitoring-plugin/tests/perses-ui-ivt/monitoring-plugin-tests-perses-ui-ivt-ref.yaml
- ci-operator/step-registry/monitoring-plugin/tests/perses-dev-ui/monitoring-plugin-tests-perses-dev-ui-commands.sh
- ci-operator/config/openshift/monitoring-plugin/openshift-monitoring-plugin-main.yaml
79aac8f to
97b1fff
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ci-operator/step-registry/monitoring-plugin/tests/perses-ui-ivt/monitoring-plugin-tests-perses-ui-ivt-commands.sh (1)
27-34: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winUse a default for indirect expansion under
nounset.With
set -o nounset(line 3),${!var}on line 28 will abort when any targetCYPRESS_*variable is completely unset (not just empty), preventing the cleanup loop from functioning as intended.Proposed fix
for var in "${vars[@]}"; do - if [[ -z "${!var}" ]]; then + if [[ -z "${!var:-}" ]]; then unset "$var"🤖 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 `@ci-operator/step-registry/monitoring-plugin/tests/perses-ui-ivt/monitoring-plugin-tests-perses-ui-ivt-commands.sh` around lines 27 - 34, The indirect variable expansion ${!var} used in the conditional check on line 28 will cause the script to abort when any CYPRESS_* variable is completely unset due to the nounset setting at line 3. Fix this by applying a default value to the indirect expansion syntax (such as ${!var:-}) when checking if the variable is empty in the if condition, allowing the cleanup loop to safely handle unset variables without aborting the script.
🤖 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
`@ci-operator/step-registry/monitoring-plugin/tests/perses-ui-ivt/monitoring-plugin-tests-perses-ui-ivt-commands.sh`:
- Around line 87-91: The CYPRESS_LOGIN_USERS export statement references an
undefined users variable which will cause script failure under set -o nounset.
Remove the undefined ${users} reference and only include the kubeadmin
authentication in the CYPRESS_LOGIN_USERS assignment. Additionally, the
CYPRESS_LOGIN_IDP_DEV_USER variable references an unconfigured
uiauto-htpasswd-idp identity provider, so either remove this export entirely or
replace it with an identity provider that is actually configured in the script.
---
Duplicate comments:
In
`@ci-operator/step-registry/monitoring-plugin/tests/perses-ui-ivt/monitoring-plugin-tests-perses-ui-ivt-commands.sh`:
- Around line 27-34: The indirect variable expansion ${!var} used in the
conditional check on line 28 will cause the script to abort when any CYPRESS_*
variable is completely unset due to the nounset setting at line 3. Fix this by
applying a default value to the indirect expansion syntax (such as ${!var:-})
when checking if the variable is empty in the if condition, allowing the cleanup
loop to safely handle unset variables without aborting the script.
🪄 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: 1c1081a9-b807-436e-967a-3ca676998a99
⛔ Files ignored due to path filters (2)
ci-operator/jobs/openshift/monitoring-plugin/openshift-monitoring-plugin-main-periodics.yamlis excluded by!ci-operator/jobs/**ci-operator/jobs/openshift/monitoring-plugin/openshift-monitoring-plugin-main-presubmits.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (10)
ci-operator/config/openshift/monitoring-plugin/openshift-monitoring-plugin-main.yamlci-operator/config/openshift/monitoring-plugin/openshift-monitoring-plugin-main__periodics.yamlci-operator/step-registry/monitoring-plugin/tests/perses-dev-ui/monitoring-plugin-tests-perses-dev-ui-commands.shci-operator/step-registry/monitoring-plugin/tests/perses-dev-ui/monitoring-plugin-tests-perses-dev-ui-ref.yamlci-operator/step-registry/monitoring-plugin/tests/perses-ui-ivt/OWNERSci-operator/step-registry/monitoring-plugin/tests/perses-ui-ivt/monitoring-plugin-tests-perses-ui-ivt-commands.shci-operator/step-registry/monitoring-plugin/tests/perses-ui-ivt/monitoring-plugin-tests-perses-ui-ivt-ref.metadata.jsonci-operator/step-registry/monitoring-plugin/tests/perses-ui-ivt/monitoring-plugin-tests-perses-ui-ivt-ref.yamlci-operator/step-registry/monitoring-plugin/tests/perses-ui/monitoring-plugin-tests-perses-ui-commands.shci-operator/step-registry/monitoring-plugin/tests/perses-ui/monitoring-plugin-tests-perses-ui-ref.yaml
💤 Files with no reviewable changes (1)
- ci-operator/step-registry/monitoring-plugin/tests/perses-ui/monitoring-plugin-tests-perses-ui-commands.sh
✅ Files skipped from review due to trivial changes (4)
- ci-operator/step-registry/monitoring-plugin/tests/perses-dev-ui/monitoring-plugin-tests-perses-dev-ui-ref.yaml
- ci-operator/step-registry/monitoring-plugin/tests/perses-ui/monitoring-plugin-tests-perses-ui-ref.yaml
- ci-operator/step-registry/monitoring-plugin/tests/perses-dev-ui/monitoring-plugin-tests-perses-dev-ui-commands.sh
- ci-operator/step-registry/monitoring-plugin/tests/perses-ui-ivt/monitoring-plugin-tests-perses-ui-ivt-ref.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- ci-operator/config/openshift/monitoring-plugin/openshift-monitoring-plugin-main__periodics.yaml
- ci-operator/step-registry/monitoring-plugin/tests/perses-ui-ivt/monitoring-plugin-tests-perses-ui-ivt-ref.metadata.json
- ci-operator/config/openshift/monitoring-plugin/openshift-monitoring-plugin-main.yaml
97b1fff to
1664377
Compare
|
[REHEARSALNOTIFIER]
Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
@etmurasaki: all tests passed! 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. |
Summary by CodeRabbit
This PR updates OpenShift CI configuration for the Observability “Monitoring Plugin” Perses UI Cypress test coverage.
CYPRESS_LOGIN_USERSset used by the Cypress suite.monitoring-plugin-tests-perses-ui-ivt/e2e-perses-ivt) including:oauthClusterOperator with anHTPasswdidentity provider, waits for OAuth to converge, configures Cypress (kubeconfig/base URL and login env vars), and collects Cypress artifacts/screenshots/videos on exit