Draft: feat(tkn): add compute-families param to remaining AWS Tekton task templates - #882
Open
amastbau wants to merge 6 commits into
Open
Draft: feat(tkn): add compute-families param to remaining AWS Tekton task templates#882amastbau wants to merge 6 commits into
amastbau wants to merge 6 commits into
Conversation
Extends the compute-families param (added in redhat-developer#872 for RHEL and SNC) to the remaining 6 AWS task templates for API consistency. - fedora, rhel-ai: param definition + conditional pass in else branch of compute-sizes check (same pattern as infra-aws-rhel.yaml) - windows-server, mac, eks, kind: param definition only (no-op; these targets do not use the standard compute-request/allocation flow) Regenerated tkn/*.yaml via make tkn-update. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughSummary by CodeRabbit
WalkthroughAWS Tekton tasks now support explicit compute sizes and optional AWS instance-family filtering. Compute sizes take precedence over CPU and memory settings. AWS task tests verify parameter definitions and required command flags. ChangesAWS compute sizing
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
amastbau
changed the title
![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 `@tkn/template/infra-aws-fedora.yaml`:
- Around line 261-263: Remove the shared eval-based command construction in
tkn/template/infra-aws-fedora.yaml: build mapt as an argument array using set --
or direct execution, and validate evaluate-families/compute-families against the
supported family-prefix grammar before use. Regenerate tkn/infra-aws-fedora.yaml
(lines 261-263) and tkn/infra-aws-rhel-ai.yaml (lines 289-291) from the fixed
templates so both generated provisioners avoid reparsing input with eval; apply
the corresponding template change at tkn/template/infra-aws-rhel-ai.yaml (lines
289-291).
🪄 Autofix
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: Pro Plus
Run ID: 4af1a5d1-5447-4d0e-b455-9ce78fad352c
📒 Files selected for processing (12)
tkn/infra-aws-eks.yamltkn/infra-aws-fedora.yamltkn/infra-aws-kind.yamltkn/infra-aws-mac.yamltkn/infra-aws-rhel-ai.yamltkn/infra-aws-windows-server.yamltkn/template/infra-aws-eks.yamltkn/template/infra-aws-fedora.yamltkn/template/infra-aws-kind.yamltkn/template/infra-aws-mac.yamltkn/template/infra-aws-rhel-ai.yamltkn/template/infra-aws-windows-server.yaml
Two table-driven tests scanning both tkn/ and tkn/template/: - TestComputeFamiliesParamDefined: all 8 infra-aws-* tasks must define the compute-families param - TestComputeFamiliesPassedInScript: rhel, ocp-snc, fedora, rhel-ai must conditionally pass --compute-families in the provisioner script Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…rom mac - eks, kind, windows: add compute-sizes + cpus/memory params and wire compute-sizes/else conditional in script so --compute-families is actually passed to the CLI (all three targets support the flag) - mac: remove compute-families param — mac uses dedicated host provisioning and the CLI does not accept the flag - Update TestComputeFamiliesParamDefined to skip mac - Add eks, kind, windows to TestComputeFamiliesPassedInScript Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
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 `@tkn/template/infra-aws-eks.yaml`:
- Around line 231-239: Remove eval-based parsing from the EKS and Windows
compute argument handling, including tkn/template/infra-aws-eks.yaml lines
231-239 and tkn/template/infra-aws-windows-server.yaml lines 249-257. Pass
interpolated task parameters through environment variables and construct a
quoted argument vector directly, applying this to all existing interpolated
parameters. Regenerate the corresponding tasks in tkn/infra-aws-eks.yaml lines
231-239 and tkn/infra-aws-windows-server.yaml lines 249-257 so they contain the
same safe handling.
In `@tkn/template/infra-aws-windows-server.yaml`:
- Around line 118-120: Update the memory default documentation in
tkn/template/infra-aws-windows-server.yaml at lines 118-120 to match the
selected default value of 200 GiB. Regenerate tkn/infra-aws-windows-server.yaml
at lines 118-120 from the corrected template; no separate manual logic change is
needed there.
🪄 Autofix
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: Pro Plus
Run ID: 573dc603-203e-4da3-89b6-25f9e69068c3
📒 Files selected for processing (7)
pkg/tkn/compute_families_test.gotkn/infra-aws-eks.yamltkn/infra-aws-kind.yamltkn/infra-aws-windows-server.yamltkn/template/infra-aws-eks.yamltkn/template/infra-aws-kind.yamltkn/template/infra-aws-windows-server.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- tkn/infra-aws-kind.yaml
- tkn/template/infra-aws-kind.yaml
Description said "default 64" but default is 200 GiB (Windows bare metal needs more RAM than standard Linux targets). Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…dows Params passed through eval must be single-quoted to prevent shell reinterpretation of special characters. fedora and rhel-ai already used quotes; apply the same pattern to the newly added compute-sizes/families blocks in eks, kind, and windows-server. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
CLI documents: "Empty means no restriction" — passing --compute-families '' is equivalent to omitting the flag. The if != "" guard was unnecessary and inconsistent with how other optional params (e.g. --gpu-manufacturer) are passed unconditionally. Remove the guard from all 7 affected templates. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tkn/infra-aws-ocp-snc.yaml (1)
274-274: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd an explicit-empty
compute-familiesregression test.All four tasks pass
--compute-families ''.pflagand Viper produce an empty slice, and the AWS selector treats it as no restriction. Extendpkg/tkn/compute_families_test.goto cover the default and template OCP-SNC and RHEL create paths.🤖 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 `@tkn/infra-aws-ocp-snc.yaml` at line 274, Add explicit-empty compute-families regression coverage in pkg/tkn/compute_families_test.go for the default and template OCP-SNC and RHEL create paths, verifying each passes --compute-families '' and preserves unrestricted AWS selection. The affected command sites are tkn/infra-aws-ocp-snc.yaml:274-274, tkn/template/infra-aws-ocp-snc.yaml:274-274, tkn/infra-aws-rhel.yaml:283-283, and tkn/template/infra-aws-rhel.yaml:283-283; no direct changes are required there.
🤖 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 `@tkn/infra-aws-ocp-snc.yaml`:
- Line 274: Replace string-built commands and eval execution at
tkn/infra-aws-ocp-snc.yaml:274-274, tkn/template/infra-aws-ocp-snc.yaml:274-274,
tkn/infra-aws-rhel.yaml:283-283, and tkn/template/infra-aws-rhel.yaml:283-283
with argument-array construction and direct "${cmd[@]}" invocation. Validate
params.compute-families against the documented comma-separated family-prefix
format before adding it to the arguments, then apply the same changes to both
templates and regenerate both rendered files.
---
Nitpick comments:
In `@tkn/infra-aws-ocp-snc.yaml`:
- Line 274: Add explicit-empty compute-families regression coverage in
pkg/tkn/compute_families_test.go for the default and template OCP-SNC and RHEL
create paths, verifying each passes --compute-families '' and preserves
unrestricted AWS selection. The affected command sites are
tkn/infra-aws-ocp-snc.yaml:274-274, tkn/template/infra-aws-ocp-snc.yaml:274-274,
tkn/infra-aws-rhel.yaml:283-283, and tkn/template/infra-aws-rhel.yaml:283-283;
no direct changes are required there.
🪄 Autofix
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: Pro Plus
Run ID: c6cd6431-377b-4df3-861b-d5a8fdec0fbe
📒 Files selected for processing (14)
tkn/infra-aws-eks.yamltkn/infra-aws-fedora.yamltkn/infra-aws-kind.yamltkn/infra-aws-ocp-snc.yamltkn/infra-aws-rhel-ai.yamltkn/infra-aws-rhel.yamltkn/infra-aws-windows-server.yamltkn/template/infra-aws-eks.yamltkn/template/infra-aws-fedora.yamltkn/template/infra-aws-kind.yamltkn/template/infra-aws-ocp-snc.yamltkn/template/infra-aws-rhel-ai.yamltkn/template/infra-aws-rhel.yamltkn/template/infra-aws-windows-server.yaml
🚧 Files skipped from review as they are similar to previous changes (10)
- tkn/template/infra-aws-windows-server.yaml
- tkn/infra-aws-eks.yaml
- tkn/template/infra-aws-rhel-ai.yaml
- tkn/template/infra-aws-fedora.yaml
- tkn/infra-aws-windows-server.yaml
- tkn/infra-aws-kind.yaml
- tkn/template/infra-aws-kind.yaml
- tkn/infra-aws-rhel-ai.yaml
- tkn/template/infra-aws-eks.yaml
- tkn/infra-aws-fedora.yaml
| if [[ "$(params.compute-families)" != "" ]]; then | ||
| cmd+="--compute-families '$(params.compute-families)' " | ||
| fi | ||
| cmd+="--compute-families '$(params.compute-families)' " |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
payload="m5'; printf 'INJECTED\n'; #"
cmd="printf '%s\n' --compute-families '$payload' "
if eval "$cmd" | grep -qx 'INJECTED'; then
echo "raw single-quoting is unsafe"
exit 1
fiRepository: redhat-developer/mapt
Length of output: 188
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
files=(
tkn/infra-aws-ocp-snc.yaml
tkn/template/infra-aws-ocp-snc.yaml
tkn/infra-aws-rhel.yaml
tkn/template/infra-aws-rhel.yaml
)
for f in "${files[@]}"; do
echo "===== $f ====="
wc -l "$f"
rg -n -C 8 --fixed-strings \
'cmd+="--compute-families' "$f"
rg -n -C 8 'eval|compute-families|params.compute-families' "$f"
done
echo "===== related declarations and invocation patterns ====="
rg -n -C 5 'compute-families|cmd\+=|cmd=\(|"\$\{cmd|\beval\b' \
tkn README.md .github 2>/dev/null || true
echo "===== template/generated comparison ====="
diff -u tkn/template/infra-aws-ocp-snc.yaml tkn/infra-aws-ocp-snc.yaml | head -200 || true
diff -u tkn/template/infra-aws-rhel.yaml tkn/infra-aws-rhel.yaml | head -200 || trueRepository: redhat-developer/mapt
Length of output: 50378
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
pairs = [
("tkn/template/infra-aws-ocp-snc.yaml", "tkn/infra-aws-ocp-snc.yaml", 250, 330),
("tkn/template/infra-aws-rhel.yaml", "tkn/infra-aws-rhel.yaml", 260, 325),
]
for template, generated, start, end in pairs:
a = Path(template).read_text().splitlines()
b = Path(generated).read_text().splitlines()
print(f"===== {template} vs {generated} =====")
for line in range(start, end + 1):
if a[line - 1] != b[line - 1]:
print(f"line {line}:")
print(" template :", a[line - 1])
print(" generated:", b[line - 1])
print("affected blocks equal:",
a[start - 1:end] == b[start - 1:end])
PY
echo "===== consumer references ====="
rg -n -C 5 --glob '!tkn/**' --glob '!**/*.yaml' \
'compute-families|compute_families' . 2>/dev/null || true
echo "===== focused shell behavior ====="
bash -c '
set -euo pipefail
payload="m5'\''; printf '\''INJECTED\n'\''; #"
cmd="printf "%s\n" --compute-families '\''$payload'\'' "
printf "command source: %s\n" "$cmd"
printf "eval output:\n"
eval "$cmd"
'Repository: redhat-developer/mapt
Length of output: 4754
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "===== compute-family implementation references ====="
rg -n -C 6 \
'computeFamilies|compute-families|ComputeFamil|compute family|family prefix' \
cmd pkg internal 2>/dev/null | head -300
echo "===== relevant file outlines ====="
for f in $(rg -l 'computeFamilies|compute-families|ComputeFamil' cmd pkg internal 2>/dev/null | head -20); do
echo "--- $f"
wc -l "$f"
ast-grep outline "$f" 2>/dev/null | head -80 || true
doneRepository: redhat-developer/mapt
Length of output: 17620
Replace string-based eval execution with safe argument handling.
All four sites insert $(params.compute-families) into a command string and execute it with eval "${cmd}". A crafted value executes injected shell code. Build the complete command as an argument array, validate the documented comma-separated family-prefix format, and invoke "${cmd[@]}" directly. Update both templates and regenerate both rendered files.
📍 Affects 4 files
tkn/infra-aws-ocp-snc.yaml#L274-L274(this comment)tkn/template/infra-aws-ocp-snc.yaml#L274-L274tkn/infra-aws-rhel.yaml#L283-L283tkn/template/infra-aws-rhel.yaml#L283-L283
🤖 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 `@tkn/infra-aws-ocp-snc.yaml` at line 274, Replace string-built commands and
eval execution at tkn/infra-aws-ocp-snc.yaml:274-274,
tkn/template/infra-aws-ocp-snc.yaml:274-274, tkn/infra-aws-rhel.yaml:283-283,
and tkn/template/infra-aws-rhel.yaml:283-283 with argument-array construction
and direct "${cmd[@]}" invocation. Validate params.compute-families against the
documented comma-separated family-prefix format before adding it to the
arguments, then apply the same changes to both templates and regenerate both
rendered files.
Source: Path instructions
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #872 (which added `compute-families` to RHEL and SNC tasks only).
adrianriobo asked why not all tasks — this PR extends it to all remaining AWS task templates.
`tkn/*.yaml` regenerated via `make tkn-update`.
E2E Test Results
All tests run with `--compute-families m5,m6i`, on-demand, verified via `aws ec2 describe-instances`, then destroyed.
rhel ✅
fedora ✅
rhel-ai ✅
kind ✅
eks (worker node) ✅
windows ⏭️ SKIP
Custom AMI `Windows_Server-2019-English-Full-HyperV-RHQE` not available in test account. Script wiring is identical to the other targets.
Test Plan
Follows up on adrianriobo's comment in #872
Generated with Claude Code