Repository navigation
OSAC-1675: Make e2e workflow reusable across repos - #84
Conversation
|
@eliorerz: This pull request references OSAC-1675 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eliorerz 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 |
|
Warning Review limit reached
More reviews will be available in 58 minutes and 44 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (2)
WalkthroughThe PR adds ChangesE2E Workflow Enhancements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (8 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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
🤖 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 @.github/workflows/e2e.yml:
- Around line 241-262: The SET_ARGS variable is built by string concatenation,
which causes argument injection risks (malicious keys could inject arbitrary
Helm flags) and word-splitting issues (values with spaces break into separate
arguments). Convert SET_ARGS from a string to a bash array by initializing it as
SET_ARGS=() at the start, then in the loop where you currently append to
SET_ARGS="${SET_ARGS} --set ${key}=${value}", instead append array elements
using SET_ARGS+=("--set" "${key}=${value}"), and finally when passing it to the
helm upgrade command on the line with --set flags, expand it safely as
"${SET_ARGS[@]}" instead of the unquoted ${SET_ARGS}.
🪄 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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: c5e9947e-15a9-4f50-ae5c-644059cb8da0
📒 Files selected for processing (2)
.github/workflows/e2e-test.yml.github/workflows/e2e.yml
| # Apply image overrides via helm upgrade --set flags. | ||
| # Callers pass a JSON object mapping Helm value paths to image refs, | ||
| # e.g. '{"service.images.service":"ghcr.io/repo:pr-42"}'. | ||
| # If empty or '{}', this is a no-op. | ||
| if [[ -n "${IMAGE_OVERRIDES}" ]] && [[ "${IMAGE_OVERRIDES}" != "{}" ]]; then | ||
| echo "Applying image overrides..." | ||
| SET_ARGS="" | ||
| for key in $(echo "${IMAGE_OVERRIDES}" | jq -r 'keys[]'); do | ||
| value=$(echo "${IMAGE_OVERRIDES}" | jq -r --arg k "${key}" '.[$k]') | ||
| SET_ARGS="${SET_ARGS} --set ${key}=${value}" | ||
| echo " --set ${key}=${value}" | ||
| done | ||
|
|
||
| yq -i '(.dependencies[].version) = "0.0.0"' charts/osac/Chart.yaml | ||
| helm dependency update charts/osac/ | ||
| helm upgrade osac charts/osac/ \ | ||
| --namespace "${INSTALLER_NAMESPACE}" \ | ||
| --values "${VALUES_FILE}" \ | ||
| ${SET_ARGS} \ | ||
| --timeout 10m \ | ||
| --wait | ||
| fi |
There was a problem hiding this comment.
Argument injection and word-splitting risks in Helm override parsing.
The current approach has two issues:
-
Argument injection via keys: A malicious JSON key like
{"--values /etc/passwd": "x"}would produce--set --values /etc/passwd=x, allowing arbitrary Helm flags to be injected. -
Word splitting on values: Since
${SET_ARGS}is expanded unquoted (line 259), values containing spaces break into separate arguments, causing incorrect Helm behavior.
Consider using an array to safely accumulate arguments:
🔒 Proposed fix using bash array for safe argument handling
if [[ -n "${IMAGE_OVERRIDES}" ]] && [[ "${IMAGE_OVERRIDES}" != "{}" ]]; then
echo "Applying image overrides..."
- SET_ARGS=""
- for key in $(echo "${IMAGE_OVERRIDES}" | jq -r 'keys[]'); do
- value=$(echo "${IMAGE_OVERRIDES}" | jq -r --arg k "${key}" '.[$k]')
- SET_ARGS="${SET_ARGS} --set ${key}=${value}"
- echo " --set ${key}=${value}"
+ SET_ARGS=()
+ while IFS= read -r key; do
+ value=$(echo "${IMAGE_OVERRIDES}" | jq -r --arg k "${key}" '.[$k]')
+ # Validate key doesn't start with - to prevent argument injection
+ if [[ "${key}" == -* ]]; then
+ echo "ERROR: Invalid override key '${key}' - keys cannot start with -" >&2
+ exit 1
+ fi
+ SET_ARGS+=(--set "${key}=${value}")
+ echo " --set ${key}=${value}"
- done
+ done < <(echo "${IMAGE_OVERRIDES}" | jq -r 'keys[]')
yq -i '(.dependencies[].version) = "0.0.0"' charts/osac/Chart.yaml
helm dependency update charts/osac/
helm upgrade osac charts/osac/ \
--namespace "${INSTALLER_NAMESPACE}" \
--values "${VALUES_FILE}" \
- ${SET_ARGS} \
+ "${SET_ARGS[@]}" \
--timeout 10m \
--wait
fi🤖 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 @.github/workflows/e2e.yml around lines 241 - 262, The SET_ARGS variable is
built by string concatenation, which causes argument injection risks (malicious
keys could inject arbitrary Helm flags) and word-splitting issues (values with
spaces break into separate arguments). Convert SET_ARGS from a string to a bash
array by initializing it as SET_ARGS=() at the start, then in the loop where you
currently append to SET_ARGS="${SET_ARGS} --set ${key}=${value}", instead append
array elements using SET_ARGS+=("--set" "${key}=${value}"), and finally when
passing it to the helm upgrade command on the line with --set flags, expand it
safely as "${SET_ARGS[@]}" instead of the unquoted ${SET_ARGS}.
Source: Coding guidelines
fd738a2 to
866788d
Compare
866788d to
9ff7366
Compare
Add image-overrides and installer-ref inputs to the shared e2e.yml so component repos can call it cross-repo with PR images swapped in via Helm --set flags. Add nightly schedule trigger to e2e-test.yml.
Consolidate related steps to reduce noise in the GH Actions UI: - Cluster prep: merge config check, flavor pull, stale cleanup into "Prepare cluster environment" - Boot: absorb kubeconfig ownership fix into "Boot cluster clone" - Teardown: merge clone destroy and temp file cleanup into "Teardown" Reduces step count from 13 to 10 with no behavioral changes.
9ff7366 to
6be92e7
Compare
This branch was previously deployed
2 participants
Summary
image-overridesandinstaller-refinputs to the sharede2e.ymlworkflow, enabling cross-repo callers to swap PR images via Helm--setflags and test installer branch changesTest plan
workflow_dispatchstill works with default inputs (no image overrides)image-overridesinputinstaller-refcorrectly clones the specified branch of osac-installerSummary by CodeRabbit
New Features
Improvements