Conversation
|
Skipping CI for Draft Pull Request. |
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
.openshift-ci/dispatch.sh, thecurl ... || breakpattern is invalid outside of a loop and will cause a shell error; replacebreakwith an explicitexit/returnand ensure the failure path is handled cleanly. - In
deploy_stackrox_with_roxie_compat, theretrying_kubectl ... delete configmap ...calls are not guarded with|| true, so a missing ConfigMap can fail the whole script underset -e; consider making these deletions tolerant of non-existent resources. - In
handle_trusted_ca_file,trusted_ca_as_stringis assigned without being declared local, which can unintentionally leak into the global scope; declare itlocalto avoid side effects on the surrounding script.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `.openshift-ci/dispatch.sh`, the `curl ... || break` pattern is invalid outside of a loop and will cause a shell error; replace `break` with an explicit `exit`/`return` and ensure the failure path is handled cleanly.
- In `deploy_stackrox_with_roxie_compat`, the `retrying_kubectl ... delete configmap ...` calls are not guarded with `|| true`, so a missing ConfigMap can fail the whole script under `set -e`; consider making these deletions tolerant of non-existent resources.
- In `handle_trusted_ca_file`, `trusted_ca_as_string` is assigned without being declared local, which can unintentionally leak into the global scope; declare it `local` to avoid side effects on the surrounding script.
## Individual Comments
### Comment 1
<location path=".openshift-ci/dispatch.sh" line_range="75-76" />
<code_context>
+if [[ "${USE_ROXIE_DEPLOY:-false}" != 'false' ]]; then
+ info 'Installing latest roxie release...'
+ if [[ ${USE_ROXIE_VERSION:-latest} == 'latest' ]]; then
+ USE_ROXIE_VERSION="$(curl -s -H "Authorization: token ${RHACS_BOT_GITHUB_TOKEN}" \
+ "https://api.github.com/repos/stackrox/roxie/releases/latest" | jq -r '.tag_name')"
+ fi
+ curl -o /tmp/roxie \
</code_context>
<issue_to_address>
**issue (bug_risk):** Consider handling missing/invalid GitHub token and curl/jq failures explicitly.
If `RHACS_BOT_GITHUB_TOKEN` is unset/invalid, this call silently uses unauthenticated rate limits, and any curl/jq failure will set `USE_ROXIE_VERSION` to an empty string, which then propagates into the download URL. Please either fail fast when the token is required but missing, and/or check curl/jq exit codes before using `.tag_name` (e.g. with `|| { error ...; exit 1; }`) to avoid constructing an invalid URL.
</issue_to_address>
### Comment 2
<location path=".openshift-ci/dispatch.sh" line_range="81" />
<code_context>
+ curl -o /tmp/roxie \
+ -H "Authorization: token ${RHACS_BOT_GITHUB_TOKEN}" \
+ -H "Accept:application/octet-stream"\
+ "https://github.com/stackrox/roxie/releases/download/${USE_ROXIE_VERSION}/roxie-linux-amd64" || break
+ install -b /tmp/roxie /usr/bin/
+ roxie version || { error 'roxie failed, restoring original version:'; cp /usr/bin/roxie.old /usr/bin/roxie; roxie version; }
</code_context>
<issue_to_address>
**issue (bug_risk):** `break` here may not behave as expected outside of a loop.
On curl failure, `break` will either throw a runtime error if this isn’t inside a loop, or exit an unrelated enclosing loop. If the goal is to stop the script/function, use `exit 1` or `return 1` instead for explicit, predictable control flow.
</issue_to_address>
### Comment 3
<location path=".openshift-ci/dispatch.sh" line_range="82-83" />
<code_context>
+ -H "Authorization: token ${RHACS_BOT_GITHUB_TOKEN}" \
+ -H "Accept:application/octet-stream"\
+ "https://github.com/stackrox/roxie/releases/download/${USE_ROXIE_VERSION}/roxie-linux-amd64" || break
+ install -b /tmp/roxie /usr/bin/
+ roxie version || { error 'roxie failed, restoring original version:'; cp /usr/bin/roxie.old /usr/bin/roxie; roxie version; }
+fi
+
</code_context>
<issue_to_address>
**issue (bug_risk):** The backup filename used in `install -b` does not match the file restored later.
GNU `install -b` defaults to a `~` backup suffix (e.g. `/usr/bin/roxie~`), but the rollback expects `/usr/bin/roxie.old`. Unless you pass `--suffix=.old` (or similar) to `install`, the restore command will fail because `roxie.old` is never created. Please align the backup suffix with the file used in the restore step or programmatically determine the backup name before restoring.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
/test gke-qa-e2e-tests |
|
/test gke-qa-e2e-tests |
🚀 Build Images ReadyImages are ready for commit 3329320. To use with deploy scripts: export MAIN_IMAGE_TAG=4.11.x-716-g3329320a1a |
|
/test gke-qa-e2e-tests |
|
/test gke-qa-e2e-tests |
|
/test gke-qa-e2e-tests |
|
/test gke-qa-e2e-tests |
|
/test gke-qa-e2e-tests |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #20111 +/- ##
==========================================
+ Coverage 49.67% 49.75% +0.08%
==========================================
Files 2765 2766 +1
Lines 209049 209549 +500
==========================================
+ Hits 103850 104267 +417
- Misses 97521 97583 +62
- Partials 7678 7699 +21
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/test gke-qa-e2e-tests |
|
/test gke-qa-e2e-tests |
|
@mclasmeier this should work if we just find a good place to put it! I wasn't considering that the test pod may have paths locked down like this (I was sure I could install to /usr/local/bin). I am thinking creating a .bin/ on the working directory may be better than this /tmp/roxie/ (if that works). If you have another idea and want to try it, feel free to push to this branch. |
|
@davdhacs: The following test failed, say
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. |
|
This is pretty cool, thank you @davdhacs |
auto-install latest roxie