Skip to content

Conversation

@yapei
Copy link
Contributor

@yapei yapei commented Nov 26, 2025

Image pull secrets.Image pull secrets Creates, edits, and deletes an image registry credentials pull secret is failing frequently due to the fact that secret data was already hidden upon checking secret data

Screenshot 2025-11-26 at 3 02 16 PM

@openshift-ci-robot openshift-ci-robot added jira/severity-important Referenced Jira bug's severity is important for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Nov 26, 2025
@openshift-ci-robot
Copy link
Contributor

@yapei: This pull request references Jira Issue OCPBUGS-66048, which is invalid:

  • expected the bug to target the "4.21.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Image pull secrets.Image pull secrets Creates, edits, and deletes an image registry credentials pull secret is frequently failing due to the fact that secret data was hidden quite fast
Screenshot 2025-11-26 at 3 02 16 PM

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.

@coderabbitai
Copy link

coderabbitai bot commented Nov 26, 2025

Walkthrough

Moves the secret reveal action in a Cypress test from before a per-item loop to inside each iteration after key retrieval, and adds an explicit 60000ms page-load timeout to the Alertmanager edit page visit call.

Changes

Cohort / File(s) Summary
Test reveal logic restructuring
frontend/packages/integration-tests-cypress/views/secret.ts
Moved secrets.clickRevealValues() from before the loop into each iteration, called immediately after obtaining the key text. This changes the reveal timing relative to clipboard/read operations per item.
Add explicit visit timeout
frontend/packages/integration-tests-cypress/views/alertmanager.ts
visitEditPage now calls cy.visit(...) with a { timeout: 60000 } option to set an explicit page-load timeout for the edit URL.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Verify timing-sensitive behavior in secret.ts to avoid race conditions between reading key text, revealing values, and clipboard assertions.
  • Confirm the added 60000ms timeout in alertmanager.ts is appropriate and doesn't mask longer navigation issues.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

@openshift-ci openshift-ci bot requested review from rhamilto and sg00dwin November 26, 2025 07:04
@openshift-ci openshift-ci bot added the kind/cypress Related to Cypress e2e integration testing label Nov 26, 2025
@yapei yapei force-pushed the ocpbugs-66048 branch 2 times, most recently from 02da5a5 to 17a9aad Compare November 26, 2025 08:14
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
frontend/packages/integration-tests-cypress/views/alertmanager.ts (1)

83-85: Verify the necessity and appropriateness of the 60-second timeout.

A 60-second timeout is quite long for page navigation and may mask underlying performance issues or actual failures. Additionally, the relationship between this change and the PR's stated objective (fixing image pull secrets test failures) is unclear.

Consider:

  1. Whether Cypress's default command timeout already covers this duration
  2. Using explicit waits for specific elements to appear after navigation instead of a blanket timeout
  3. Investigating whether there's an underlying performance issue that should be addressed

Can you clarify how this change relates to the image pull secrets test failure described in the PR objectives? If the page consistently takes this long to load, consider investigating the root cause rather than extending the timeout.

Recommended approach using element-based wait:

 visitEditPage: (receiverName: string) => {
-  cy.visit(`/settings/cluster/alertmanagerconfig/receivers/${receiverName}/edit`, {
-    timeout: 60000,
-  });
+  cy.visit(`/settings/cluster/alertmanagerconfig/receivers/${receiverName}/edit`);
+  // Wait for the edit form to be ready instead of a blanket timeout
+  cy.byTestID('receiver-name').should('be.visible');
 },
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 38ada6a and 02da5a5.

📒 Files selected for processing (2)
  • frontend/packages/integration-tests-cypress/views/alertmanager.ts (1 hunks)
  • frontend/packages/integration-tests-cypress/views/secret.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • frontend/packages/integration-tests-cypress/views/secret.ts
🧰 Additional context used
📓 Path-based instructions (1)
**

⚙️ CodeRabbit configuration file

-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.

Files:

  • frontend/packages/integration-tests-cypress/views/alertmanager.ts

Copy link
Member

@logonoff logonoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 26, 2025
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 26, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: logonoff, yapei

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 26, 2025
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 26, 2025

@yapei: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-gcp-console 17a9aad link unknown /test e2e-gcp-console

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. jira/severity-important Referenced Jira bug's severity is important for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. kind/cypress Related to Cypress e2e integration testing lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants