OCPBUGS-89515: v1/e2e: reduce helm download flakiness#1446
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@r4f4: This pull request references CLID-667 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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:
WalkthroughA new ChangesBinary Download Helper Consolidation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
r4f4
force-pushed
the
clid-667-v1-helm-flakiness
branch
from
63e258d to
f625604
Compare
![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
🤖 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 `@v1/test/e2e/lib/util.sh`:
- Around line 48-67: The download_binary function has a critical typo on line 54
where the while loop condition references the undefined variable $max instead of
$max_attempts, causing a syntax error. Fix this by replacing $max with
$max_attempts in the while loop condition. Additionally, add the -f flag to the
curl command on line 55 to ensure that HTTP errors cause the download to fail
rather than being silently treated as successful.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1ad419e1-298d-4a5a-8cee-7f4c230d669a
📒 Files selected for processing (1)
v1/test/e2e/lib/util.sh
There was a problem hiding this comment.
♻️ Duplicate comments (1)
v1/test/e2e/lib/util.sh (1)
54-55:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix unresolved retry-loop typo and fail-fast download behavior.
Line 54 references
$maxinstead of$max_attempts, which breaks the conditional parsing; Line 55 should usecurl -fLso HTTP errors fail immediately.Suggested patch
- while [[ $attempt -le $max]]; do - if curl -L $url -o $name; then + while [[ $attempt -le $max_attempts ]]; do + if curl -fL "$url" -o "$name"; then#!/usr/bin/env bash set -euo pipefail # 1) Validate shell syntax for util.sh (should exit 0 after fix) bash -n v1/test/e2e/lib/util.sh # 2) Confirm corrected loop variable and curl flags in helper nl -ba v1/test/e2e/lib/util.sh | sed -n '48,67p'🤖 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 `@v1/test/e2e/lib/util.sh` around lines 54 - 55, In the retry loop that begins with while [[ $attempt -le, fix the variable name from $max to $max_attempts to properly reference the loop boundary condition. Additionally, in the curl command on the following line that reads curl -L $url -o $name, add the -f flag to curl (changing it to curl -fL) so that HTTP error responses are treated as failures and cause the download to fail immediately instead of silently continuing.Source: Linters/SAST tools
🤖 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.
Duplicate comments:
In `@v1/test/e2e/lib/util.sh`:
- Around line 54-55: In the retry loop that begins with while [[ $attempt -le,
fix the variable name from $max to $max_attempts to properly reference the loop
boundary condition. Additionally, in the curl command on the following line that
reads curl -L $url -o $name, add the -f flag to curl (changing it to curl -fL)
so that HTTP error responses are treated as failures and cause the download to
fail immediately instead of silently continuing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 35cc29fc-2a61-4459-9128-4cd7bf37c66a
📒 Files selected for processing (1)
v1/test/e2e/lib/util.sh
r4f4
force-pushed
the
clid-667-v1-helm-flakiness
branch
2 times, most recently
from
4c61b46 to
ca667c0
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adolfo-ab, r4f4 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 |
r4f4
force-pushed
the
clid-667-v1-helm-flakiness
branch
from
ca667c0 to
a37866b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@v1/test/e2e/lib/util.sh`:
- Around line 55-56: Unquoted variables in shell commands can cause word
splitting and globbing issues when paths or values contain whitespace or special
characters. Quote the variables `$url` and `$name` in the curl and chmod
commands on lines 55-56 by wrapping them in double quotes. Apply the same
quoting pattern to the unquoted variables on lines 85, 92, and 241 that are
passed to command invocations (such as variables used with `$GOBIN/...` paths).
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 07034998-4450-400b-93da-1505d4aed48b
📒 Files selected for processing (1)
v1/test/e2e/lib/util.sh
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 `@v1/test/e2e/lib/util.sh`:
- Line 60: The echo statement on line 60 logs the complete URL variable which
could expose sensitive information such as credentials or API keys in CI logs.
Extract and log only the binary name (e.g., the filename from the URL) or
provide a redacted version of the URL instead of the full $url variable in the
download failure message. This ensures that signed URLs with embedded
credentials remain protected while still providing useful debugging information
about which binary download failed.
- Around line 49-53: Variables `url`, `name`, `max_attempts`, and `attempt` are
declared without the `local` keyword, making them global scope and causing
potential state bleed across functions. Add the `local` keyword before each
variable declaration to properly scope them to the function where they are used.
This ensures that each function has its own isolated variable space and prevents
accidental interference with other functions.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e76a57de-15ea-40c7-857f-bb33967e536e
📒 Files selected for processing (1)
v1/test/e2e/lib/util.sh
r4f4
force-pushed
the
clid-667-v1-helm-flakiness
branch
from
a37866b to
6f0b0d6
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
r4f4
force-pushed
the
clid-667-v1-helm-flakiness
branch
2 times, most recently
from
4fe5da8 to
8607f68
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@v1/test/e2e/lib/util.sh`:
- Line 55: The curl command with flags -fL in the download operation lacks
timeout limits, allowing it to hang indefinitely on slow or stalled connections
and defeating the retry mechanism. Add timeout options to the curl invocation
(such as --connect-timeout for connection establishment and --max-time for total
operation duration) to ensure the command fails quickly when encountering
network issues, allowing retries to trigger as intended.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 90d850eb-d170-4704-b42b-5d9aac24f64e
📒 Files selected for processing (1)
v1/test/e2e/lib/util.sh
r4f4
force-pushed
the
clid-667-v1-helm-flakiness
branch
from
8607f68 to
a652916
Compare
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
When downloading binaries, attempt a few times before failing the whole e2e suite.
r4f4
force-pushed
the
clid-667-v1-helm-flakiness
branch
from
a652916 to
a893c10
Compare
|
@r4f4: 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. |
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/retitle OCPBUGS-89515: v1/e2e: reduce helm download flakiness |
openshift-ci
Bot
changed the title
openshift-ci-robot
added
the
jira/valid-bug
|
@r4f4: This pull request references Jira Issue OCPBUGS-89515, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
|
/verified bypass |
openshift-ci-robot
added
the
verified
|
@r4f4: The 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. |
|
@r4f4: Jira Issue Verification Checks: Jira Issue OCPBUGS-89515 Jira Issue OCPBUGS-89515 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 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. |
|
/cherry-pick release-4.23 |
|
@r4f4: new pull request created: #1447 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 kubernetes-sigs/prow repository. |
|
/cherry-pick release-4.22 |
|
@r4f4: new pull request created: #1455 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 kubernetes-sigs/prow repository. |
5 participants
Description
When downloading binaries, attempt a few times before failing the whole e2e suite.
Github / Jira issue: CLID-667
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration.
Expected Outcome
Please describe the outcome expected from the tests.
Summary by CodeRabbit
Note: These changes are limited to test reliability and do not affect end-user functionality.