Skip to content

test: new auth providers for e2e - #3355

Closed
jrichter1 wants to merge 1 commit into
redhat-developer:mainfrom
jrichter1:auth-test
Closed

test: new auth providers for e2e#3355
jrichter1 wants to merge 1 commit into
redhat-developer:mainfrom
jrichter1:auth-test

Conversation

@jrichter1

@jrichter1 jrichter1 commented Aug 21, 2026

Copy link
Copy Markdown
Member

just testing changes for the utils

@jrichter1
jrichter1 requested review from a team, gashcrumb and kadel as code owners August 21, 2026 10:09
@jrichter1

Copy link
Copy Markdown
Member Author

/test e2e-helm-ocp-nightly

@sonarqubecloud

Copy link
Copy Markdown

@jrichter1

Copy link
Copy Markdown
Member Author

/test e2e-ocp-helm-nightly

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 21, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 10:11 AM UTC · Completed 10:14 AM UTC

Commit: 4cbaab8 · View workflow run →

@fullsend-ai-review

Copy link
Copy Markdown

Review — ⛔ Changes Requested

PR: #3355test: new auth providers for e2e
Author: jrichter1 (member)
Scope: 1 file changed (run-e2e.sh), 1 addition, 1 deletion

Summary

This PR replaces the environment-variable-driven default for E2E_TEST_UTILS_GIT_REF with a hardcoded reference to a personal fork branch (jrichter1/rhdh-e2e-test-utils#auth-providers). The PR description says "just testing changes for the utils," confirming this is intended for local experimentation — but the PR targets main and is not marked as a draft.

Findings

🔴 High — Hardcoded personal fork breaks all E2E infrastructure

File: run-e2e.sh, line 82

The change replaces:

E2E_TEST_UTILS_GIT_REF="${E2E_TEST_UTILS_GIT_REF:-}"

with:

E2E_TEST_UTILS_GIT_REF="jrichter1/rhdh-e2e-test-utils#auth-providers"

Impact if merged:

  • All E2E runs (nightly CI, PR checks, local dev via run-e2e.sh) would clone and build from a personal fork branch instead of using the published @red-hat-developer-hub/e2e-test-utils npm package.
  • The environment variable override is completely removed — setting E2E_TEST_UTILS_GIT_REF externally has no effect since the script no longer reads from the environment (no ${...:-} expansion).
  • The E2E_TEST_UTILS_PATH and E2E_TEST_UTILS_VERSION configuration paths (lines 78–80) become dead code: the hardcoded git ref always triggers the clone block (lines 84–90), which unconditionally overwrites E2E_TEST_UTILS_PATH.
  • CI reliability becomes dependent on the availability and state of a personal fork branch that could be force-pushed, deleted, or diverge from the upstream API at any time.

Remediation: If the intent is to test changes to rhdh-e2e-test-utils, use the existing env variable mechanism locally or in a PR check:

E2E_TEST_UTILS_GIT_REF="jrichter1/rhdh-e2e-test-utils#auth-providers" ./run-e2e.sh -w <workspace>

Do not commit this hardcoded value to main. If this PR is meant purely for CI experimentation (e.g., triggering a /test run with the fork), consider marking it as a draft or adding the do-not-merge/work-in-progress label.

Verdict

request-changes — This change must not be merged to main. It hardcodes a personal fork reference into the shared E2E test runner, which would break the environment variable configuration contract and make all E2E runs dependent on an external personal repository.


Labels: PR modifies the E2E test runner script and appears to be a work-in-progress test change

@fullsend-ai-review fullsend-ai-review Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

See the review comment for full details.

Comment thread run-e2e.sh
E2E_TEST_UTILS_VERSION="${E2E_TEST_UTILS_VERSION:-}"
# Git ref for e2e-test-utils: "owner/repo#branch" — clones and sets E2E_TEST_UTILS_PATH
E2E_TEST_UTILS_GIT_REF="${E2E_TEST_UTILS_GIT_REF:-}"
E2E_TEST_UTILS_GIT_REF="jrichter1/rhdh-e2e-test-utils#auth-providers"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[high] correctness

E2E_TEST_UTILS_GIT_REF is hardcoded to a personal fork branch (jrichter1/rhdh-e2e-test-utils#auth-providers), replacing the environment variable override pattern. If merged, all E2E runs (nightly, PR checks, local) would clone from a personal fork instead of using the published npm package. The env var override is removed, and E2E_TEST_UTILS_PATH/E2E_TEST_UTILS_VERSION settings become dead code.

Suggested fix: Revert to the original pattern: E2E_TEST_UTILS_GIT_REF="${E2E_TEST_UTILS_GIT_REF:-}". Use the env var mechanism to test fork branches locally or in CI without committing the reference.

@fullsend-ai-review fullsend-ai-review Bot added do-not-merge/work-in-progress non-workspace-changes PR changes files outside workspace directories labels Aug 21, 2026
@rhdh-test-bot

Copy link
Copy Markdown

❌ Failed Nightly E2E Tests

Platform: ocp 4.20 | RHDH Version: 1.11 | Duration: 45m 2s
Passed: 208 | Failed: 17 | Flaky: 0 | Skipped: 66
Playwright Report | Build Log | Logs | Artifacts

@jrichter1

Copy link
Copy Markdown
Member Author

/test e2e-ocp-helm-nightly

@rhdh-test-bot

Copy link
Copy Markdown

❌ Failed Nightly E2E Tests

Platform: ocp 4.20 | RHDH Version: 1.11 | Duration: 47m 26s
Passed: 204 | Failed: 17 | Flaky: 0 | Skipped: 71
Playwright Report | Build Log | Logs | Artifacts

@jrichter1

Copy link
Copy Markdown
Member Author

/test e2e-ocp-helm-nightly

@rhdh-test-bot

Copy link
Copy Markdown

❌ Failed Nightly E2E Tests

Platform: ocp 4.20 | RHDH Version: 1.11 | Duration: 45m 25s
Passed: 212 | Failed: 9 | Flaky: 0 | Skipped: 71
Playwright Report | Build Log | Logs | Artifacts

@openshift-ci

openshift-ci Bot commented Aug 21, 2026

Copy link
Copy Markdown

@jrichter1: 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-ocp-helm-nightly 4cbaab8 link false /test e2e-ocp-helm-nightly

Full PR test history. Your PR dashboard.

Details

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.

@jrichter1 jrichter1 closed this Aug 21, 2026
@fullsend-ai-retro

fullsend-ai-retro Bot commented Aug 21, 2026

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 1:44 PM UTC · Completed 1:55 PM UTC

Commit: 4cbaab8 · View workflow run →

@fullsend-ai-retro

Copy link
Copy Markdown

Retro: PR #3355 — test: new auth providers for e2e

Verdict: Workflow operated correctly. No novel proposals warranted.

Timeline

PR #3355 was a short-lived human-authored WIP PR by jrichter1, opened and closed on 2026-08-21 without merging. It hardcoded E2E_TEST_UTILS_GIT_REF in run-e2e.sh to point at a personal fork (jrichter1/rhdh-e2e-test-utils#auth-providers) to test auth provider changes for E2E utilities. The PR was a deliberate test vehicle for work in the companion rhdh-e2e-test-utils PR #150.

What went well

  • Review agent correctly identified the hardcoded personal fork reference as a high-severity correctness issue, requested changes, and applied the do-not-merge/work-in-progress label — all within ~2 minutes at $0.51 cost. The review was accurate, actionable, and appropriately calibrated.
  • Routing worked correctly: the opened event dispatched to review, the agent's own pull_request_review event was correctly no-oped, and the closed event dispatched to retro.
  • Label application was appropriate: the agent deduced WIP intent from the PR description and diff content even though the PR was not a draft and had no labels at open time.

Evidence for existing issues

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

Labels

do-not-merge/work-in-progress non-workspace-changes PR changes files outside workspace directories

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants