fix: make PODVM_INSTANCE_TYPES configurable in peerpods-param-cm step#79581
fix: make PODVM_INSTANCE_TYPES configurable in peerpods-param-cm step#79581thejasn wants to merge 2 commits into
Conversation
WalkthroughThe ref manifest declares an env var ChangesConfigurable PODVM Instance Types
🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ci-operator/step-registry/sandboxed-containers-operator/peerpods/param-cm/sandboxed-containers-operator-peerpods-param-cm-commands.sh (1)
1-1:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAdd required
set -euo pipefailat the beginning of the script.This script is missing the required error-handling flags. As per coding guidelines, step registry script files must use
set -euo pipefail(without-x) as default. Without these flags, command failures may be silently ignored (e.g., the AWS CLI calls on lines 40-44 or theoccommands), leading to subtle bugs.🛡️ Proposed fix
#!/bin/bash +set -euo pipefailAs per coding guidelines: "Step registry script files must use
set -euo pipefail(without-x) as default and only enable-xwhen actively debugging"🤖 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 `@ci-operator/step-registry/sandboxed-containers-operator/peerpods/param-cm/sandboxed-containers-operator-peerpods-param-cm-commands.sh` at line 1, Add the required strict-shell flags to the top of the script: immediately after the existing shebang (#!/bin/bash) insert a single line "set -euo pipefail" (do not add -x). This ensures failures in subsequent commands (including the AWS CLI calls and the oc commands referenced in the script) cause the script to exit and prevents silent failures.
🤖 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.
Outside diff comments:
In
`@ci-operator/step-registry/sandboxed-containers-operator/peerpods/param-cm/sandboxed-containers-operator-peerpods-param-cm-commands.sh`:
- Line 1: Add the required strict-shell flags to the top of the script:
immediately after the existing shebang (#!/bin/bash) insert a single line "set
-euo pipefail" (do not add -x). This ensures failures in subsequent commands
(including the AWS CLI calls and the oc commands referenced in the script) cause
the script to exit and prevents silent failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 4215aa55-4966-42f3-8d1d-d6fabe6aa442
📒 Files selected for processing (2)
ci-operator/step-registry/sandboxed-containers-operator/peerpods/param-cm/sandboxed-containers-operator-peerpods-param-cm-commands.shci-operator/step-registry/sandboxed-containers-operator/peerpods/param-cm/sandboxed-containers-operator-peerpods-param-cm-ref.yaml
|
LGTM |
ldoktor
left a comment
ldoktor
left a comment
There was a problem hiding this comment.
Hello @thejasn I like your change, could you please separate the value change (removal of p3.2xlarge) and the introduction of the PODVM_INSTANCE_TYPES variable so in case of a revert or investigation of why things changed it's easily bisectable?
One more thing to consider is the defaults handling. I'd suggest having it only on one place, therefore remove it from the commands.sh as it will be set via the yaml file. That way we have only one place to modify and they will be always in sync. Another acceptable solution would be to set PODVM_INSTANCES_TYPES to "" in the yaml file and rely on commands.sh defaults. But I'd really like to see just one place to configure this value.
Signed-off-by: Thejas N <thn@redhat.com>
thejasn
force-pushed
the
thn/osc-podvm-instance-types
branch
from
3d2dd5e to
b8eb232
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ci-operator/step-registry/sandboxed-containers-operator/peerpods/param-cm/sandboxed-containers-operator-peerpods-param-cm-commands.sh (1)
1-2: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd required error handling:
set -euo pipefailStep registry command scripts must include
set -euo pipefail(without-x) after the shebang to ensure proper error handling: exit on undefined variables, command failures, and pipe failures.🛡️ Proposed fix
#!/bin/bash +set -euo pipefailAs per coding guidelines: Step registry script files must use
set -euo pipefail(without-x) as default and only enable-xwhen actively debugging.🤖 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 `@ci-operator/step-registry/sandboxed-containers-operator/peerpods/param-cm/sandboxed-containers-operator-peerpods-param-cm-commands.sh` around lines 1 - 2, The script currently only has the shebang (#!/bin/bash) and is missing robust error handling; immediately after the shebang add the required shell options by enabling errexit, nounset and pipefail (i.e., use set -euo pipefail without -x) so the script exits on command failures, undefined variables, and pipe errors; update the top of the sandboxed-containers-operator-peerpods-param-cm-commands.sh script by inserting this set invocation directly after the existing #! /bin/bash line.
🤖 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.
Outside diff comments:
In
`@ci-operator/step-registry/sandboxed-containers-operator/peerpods/param-cm/sandboxed-containers-operator-peerpods-param-cm-commands.sh`:
- Around line 1-2: The script currently only has the shebang (#!/bin/bash) and
is missing robust error handling; immediately after the shebang add the required
shell options by enabling errexit, nounset and pipefail (i.e., use set -euo
pipefail without -x) so the script exits on command failures, undefined
variables, and pipe errors; update the top of the
sandboxed-containers-operator-peerpods-param-cm-commands.sh script by inserting
this set invocation directly after the existing #! /bin/bash line.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: efb16548-5dad-43f6-b2f4-f3c7bc97a6e6
📒 Files selected for processing (2)
ci-operator/step-registry/sandboxed-containers-operator/peerpods/param-cm/sandboxed-containers-operator-peerpods-param-cm-commands.shci-operator/step-registry/sandboxed-containers-operator/peerpods/param-cm/sandboxed-containers-operator-peerpods-param-cm-ref.yaml
|
/pj-rehearse periodic-ci-openshift-sandboxed-containers-operator-devel-downstream-candidate420-aws-ipi-peerpods |
|
@ldoktor: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
openshift-ci
Bot
added
lgtm
Replace the hardcoded list of AWS instance types with an env var
${PODVM_INSTANCE_TYPES:-<defaults>} so callers can override it.
The default retains all original types except p3.2xlarge, which
is not available in all regions. CAA validates all entries at
startup, so only types available in the target region should be
included.
Signed-off-by: Thejas N <thn@redhat.com>
thejasn
force-pushed
the
thn/osc-podvm-instance-types
branch
from
b8eb232 to
f32aa65
Compare
|
/pj-rehearse periodic-ci-openshift-sandboxed-containers-operator-devel-downstream-candidate420-aws-ipi-peerpods |
|
@thejasn: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
[REHEARSALNOTIFIER]
A total of 49 jobs have been affected by this change. The above listing is non-exhaustive and limited to 25 jobs. A full list of affected jobs can be found here Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ci-operator/step-registry/sandboxed-containers-operator/peerpods/param-cm/sandboxed-containers-operator-peerpods-param-cm-commands.sh (1)
1-1: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd required
set -euo pipefailat script start.Step registry command scripts must use
set -euo pipefail(without-x) as default. This pre-existing omission reduces error detection:-ewould halt on command failures,-uwould catch unset variable references, and-o pipefailwould detect failures in pipelines.As per coding guidelines, step registry scripts must follow this pattern.
🛡️ Proposed fix
#!/bin/bash +set -euo pipefail if [ "$ENABLEPEERPODS" != "true" ]; thenNote: After adding
set -u, verify that all variable references are either guaranteed to be set or use the${VAR:-default}pattern. In this case,PODVM_INSTANCE_TYPESis set by the ref file's default (line 19 in the .yaml), so line 72's${PODVM_INSTANCE_TYPES}will work correctly.🤖 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 `@ci-operator/step-registry/sandboxed-containers-operator/peerpods/param-cm/sandboxed-containers-operator-peerpods-param-cm-commands.sh` at line 1, Add "set -euo pipefail" as the very first executable line of the script (without "-x") to ensure the script exits on errors, treats unset variables as errors, and catches pipeline failures; after adding it, audit any variable usages and replace any potentially unset references with a safe default or explicit check (e.g., use ${VAR:-default}) where needed — specifically verify the use of PODVM_INSTANCE_TYPES in the script is safe (it is expected to be provided by the ref file) and adjust any other variable references accordingly.
🤖 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.
Outside diff comments:
In
`@ci-operator/step-registry/sandboxed-containers-operator/peerpods/param-cm/sandboxed-containers-operator-peerpods-param-cm-commands.sh`:
- Line 1: Add "set -euo pipefail" as the very first executable line of the
script (without "-x") to ensure the script exits on errors, treats unset
variables as errors, and catches pipeline failures; after adding it, audit any
variable usages and replace any potentially unset references with a safe default
or explicit check (e.g., use ${VAR:-default}) where needed — specifically verify
the use of PODVM_INSTANCE_TYPES in the script is safe (it is expected to be
provided by the ref file) and adjust any other variable references accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ae630367-38f5-427b-b103-9d3ee86383b2
📒 Files selected for processing (2)
ci-operator/step-registry/sandboxed-containers-operator/peerpods/param-cm/sandboxed-containers-operator-peerpods-param-cm-commands.shci-operator/step-registry/sandboxed-containers-operator/peerpods/param-cm/sandboxed-containers-operator-peerpods-param-cm-ref.yaml
|
/lgtm Thanks @thejasn ! |
|
/pj-rehearse periodic-ci-openshift-sandboxed-containers-operator-devel-downstream-candidate420-aws-ipi-peerpods |
|
@wainersm: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ldoktor, thejasn, wainersm 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 |
|
@thejasn: 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. |
4 participants
Replace the hardcoded list of AWS instance types with an env var ${PODVM_INSTANCE_TYPES:-} so callers can override it. The default retains all original types except p3.2xlarge, which is not available in all regions. CAA validates all entries at startup, so only types available in the target region should be included.
Summary by CodeRabbit
This PR updates OpenShift CI configuration for the Sandboxed Containers Operator peer-pods setup by making the AWS instance-type list configurable.
What changed (practical effect)
Why it matters