fix: fix dex e2e test failures#1165
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThree E2E tests updated: two Dex client-secret tests now fetch a deterministic Dex token Secret and validate its token and expiry fields before comparing to argocd-secret, and the toolchain test updates the expected Redis version from 7.2.11 to 8.2.3. ChangesDex Token Secret Validation Pattern
Toolchain Version Update
🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 1
🧹 Nitpick comments (1)
test/openshift/e2e/ginkgo/parallel/1-031_validate_toolchain_test.go (1)
95-104: ⚡ Quick winSimplify version assignment to eliminate duplication.
Both the
prowand non-prowbranches now setexpected_redisVersionto"8.2.3". Since the values are identical, consider extracting this to a single assignment outside the conditional block to improve maintainability.♻️ Proposed refactor to eliminate duplication
var expected_dexVersion string var expected_redisVersion string + expected_redisVersion = "8.2.3" if os.Getenv("CI") == "prow" { // when running against openshift-ci expected_dexVersion = "v2.45.0" - expected_redisVersion = "8.2.3" } else { // when running against RC/ released version of gitops expected_dexVersion = "v2.45.0" - expected_redisVersion = "8.2.3" }🤖 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 `@test/openshift/e2e/ginkgo/parallel/1-031_validate_toolchain_test.go` around lines 95 - 104, Both branches of the CI check set identical values (expected_dexVersion and expected_redisVersion), so remove duplication by assigning the common value(s) before the conditional; for example, set expected_redisVersion = "8.2.3" (and expected_dexVersion = "v2.45.0" if both always match) above the if that checks os.Getenv("CI") == "prow", then only keep branch-specific overrides (if any) inside the if/else so functions/variables named expected_dexVersion and expected_redisVersion are initialized once and the conditional only contains unique logic.
🤖 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 `@test/openshift/e2e/ginkgo/parallel/1-031_validate_toolchain_test.go`:
- Line 103: The test currently fails to find a Redis pod before running the
version check; update the verification to discover the actual Redis pod using
the same selector/pod-name resolution logic used elsewhere in the test (e.g.,
look for pods with label app.kubernetes.io/name=redis or any pod name containing
"redis" in the openshift-gitops namespace), then exec into that pod and run
`redis-server -v`; parse the output to extract the v=X.Y.Z token and assert it
equals the expected_redisVersion ("8.2.3") and that the full output matches the
expected "Redis server v=X.X.X sha=..." format used by the parsing code around
lines 167-173 so the check reliably targets the real Redis container before
parsing.
---
Nitpick comments:
In `@test/openshift/e2e/ginkgo/parallel/1-031_validate_toolchain_test.go`:
- Around line 95-104: Both branches of the CI check set identical values
(expected_dexVersion and expected_redisVersion), so remove duplication by
assigning the common value(s) before the conditional; for example, set
expected_redisVersion = "8.2.3" (and expected_dexVersion = "v2.45.0" if both
always match) above the if that checks os.Getenv("CI") == "prow", then only keep
branch-specific overrides (if any) inside the if/else so functions/variables
named expected_dexVersion and expected_redisVersion are initialized once and the
conditional only contains unique logic.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 923832f2-925c-477d-95a5-fa8d4e1a433b
📒 Files selected for processing (3)
test/openshift/e2e/ginkgo/parallel/1-031_validate_toolchain_test.gotest/openshift/e2e/ginkgo/parallel/1-095_validate_dex_clientsecret_test.gotest/openshift/e2e/ginkgo/parallel/1-098_validate_dex_clientsecret_deprecated.go
| // when running against RC/ released version of gitops | ||
| expected_dexVersion = "v2.45.0" | ||
| expected_redisVersion = "7.2.11" | ||
| expected_redisVersion = "8.2.3" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify actual Redis version and output format in openshift-gitops namespace
# Wait for Redis pod to be ready
echo "Waiting for Redis pod..."
redis_pod=$(oc get pods -n openshift-gitops -l app.kubernetes.io/name=redis -o jsonpath='{.items[0].metadata.name}' 2>/dev/null)
if [ -z "$redis_pod" ]; then
echo "ERROR: No Redis pod found in openshift-gitops namespace"
exit 1
fi
echo "Found Redis pod: $redis_pod"
# Get Redis version
echo "Extracting Redis version..."
version_output=$(oc exec -n openshift-gitops "$redis_pod" -- redis-server -v 2>/dev/null)
echo "Raw output: $version_output"
# Check if output matches expected format
if echo "$version_output" | grep -q "v="; then
version=$(echo "$version_output" | sed 's/.*v=\([^ ]*\).*/\1/')
echo "Parsed version: $version"
echo "Expected version: 8.2.3"
if [ "$version" = "8.2.3" ]; then
echo "✓ Version matches expectation"
else
echo "✗ Version mismatch: got $version, expected 8.2.3"
fi
else
echo "✗ Output format does not contain 'v=' pattern"
echo "This may break the version parsing logic"
fiRepository: redhat-developer/gitops-operator
Length of output: 157
Fix Redis-version verification to locate the actual Redis pod before checking output format.
The suggested verification script doesn’t find any Redis pod in the openshift-gitops namespace with -l app.kubernetes.io/name=redis (“No Redis pod found…”). Update the script to discover the Redis pod using the same selector/pod name logic the test uses (or dynamically search pod names for “redis” in the relevant namespace(s)), then run redis-server -v and ensure the parsed v=X.Y.Z matches 8.2.3 and conforms to the expected Redis server v=X.X.X sha=... format used by the parsing logic (around lines 167-173).
🤖 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 `@test/openshift/e2e/ginkgo/parallel/1-031_validate_toolchain_test.go` at line
103, The test currently fails to find a Redis pod before running the version
check; update the verification to discover the actual Redis pod using the same
selector/pod-name resolution logic used elsewhere in the test (e.g., look for
pods with label app.kubernetes.io/name=redis or any pod name containing "redis"
in the openshift-gitops namespace), then exec into that pod and run
`redis-server -v`; parse the output to extract the v=X.Y.Z token and assert it
equals the expected_redisVersion ("8.2.3") and that the full output matches the
expected "Redis server v=X.X.X sha=..." format used by the parsing code around
lines 167-173 so the check reliably targets the real Redis container before
parsing.
There was a problem hiding this comment.
We might not require this change. Please revert if the tests fail.
There was a problem hiding this comment.
@svghadi, understood. The change aligns both the prow and non-prow paths to 8.2.3, so monitoring the test results will be the clearest signal. If the deployed Redis image reports a different version string, reverting this line should be straightforward. Feel free to ping me if you'd like help investigating a failure.
Signed-off-by: Rizwana777 <rizwananaaz177@gmail.com>
Rizwana777
force-pushed
the
fix-dex-e2e-failures
branch
from
9649256 to
64672ad
Compare
|
/retest |
|
/test v4.19-kuttl-sequential |
2 participants
What type of PR is this?
What does this PR do / why we need it:
Have you updated the necessary documentation?
Which issue(s) this PR fixes:
Fixes #?
Test acceptance criteria:
How to test changes / Special notes to the reviewer: