ci: deduplicate check-params-env scripts into one#3334
ci: deduplicate check-params-env scripts into one#3334
Conversation
github-actions
Bot
added
the
review-requested
📝 WalkthroughWalkthroughConsolidated per-variant CI validation into a single script Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Critical Review Items
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3334 +/- ##
=====================================
Coverage 3.50% 3.50%
=====================================
Files 30 30
Lines 3395 3395
Branches 543 543
=====================================
Hits 119 119
Misses 3274 3274
Partials 2 2
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ci/check-params-env.sh`:
- Around line 654-678: check_image_repo_name() only strips "-c9s" from
image_variable but not from repository_name, causing false mismatches for repo
values like "odh-notebook-rstudio-server-c9s-python-3.12"; update
check_image_repo_name() to normalize both sides by removing the "-c9s" suffix
(or equivalent marker) from repository_name as well as image_variable before
comparison, so the expected_name/expected_repo checks for entries such as
odh-notebook-rstudio-server-c9s-python-3.12 pass correctly.
- Around line 26-44: The merged validator script lacks strict shell options, so
failures in file access or any stage of the sed|grep|jq pipelines can be masked;
add "set -euo pipefail" near the top of ci/check-params-env.sh (immediately
after the shebang) to enable exit-on-error, treat unset variables as errors, and
make pipelines fail on any stage; this will make validation of paths like
COMMIT_LATEST_ENV_PATH, PARAMS_ENV_PATH and the conditional branch on KONFLUX
robust and fail fast on missing files or broken pipeline commands.
🪄 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 YAML (base), Central YAML (inherited), Repository UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: afc2b214-5e1e-4cc8-a936-1d3be03fab7b
📒 Files selected for processing (3)
.github/workflows/params-env.yamlci/check-params-env-odh.shci/check-params-env.sh
💤 Files with no reviewable changes (1)
- ci/check-params-env-odh.sh
| # When KONFLUX=yes, validate the RHOAI manifests; otherwise default to ODH. | ||
| if [ "${KONFLUX:-no}" = 'yes' ]; then | ||
| _MANIFESTS_VARIANT="rhoai" | ||
| # This value needs to be updated everytime we deliberately change number of the | ||
| # images we want to have in the `params.env` or `params-latest.env` file. | ||
| EXPECTED_COMMIT_NUM_RECORDS=74 | ||
| EXPECTED_PARAMS_NUM_RECORDS=74 | ||
| else | ||
| _MANIFESTS_VARIANT="odh" | ||
| # This value needs to be updated everytime we deliberately change number of the | ||
| # images we want to have in the `params.env` or `params-latest.env` file. | ||
| EXPECTED_COMMIT_NUM_RECORDS=39 | ||
| EXPECTED_PARAMS_NUM_RECORDS=33 | ||
| fi | ||
|
|
||
| COMMIT_LATEST_ENV_PATH="manifests/${_MANIFESTS_VARIANT}/base/commit-latest.env" | ||
| COMMIT_ENV_PATH="manifests/${_MANIFESTS_VARIANT}/base/commit.env" | ||
| PARAMS_LATEST_ENV_PATH="manifests/${_MANIFESTS_VARIANT}/base/params-latest.env" | ||
| PARAMS_ENV_PATH="manifests/${_MANIFESTS_VARIANT}/base/params.env" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add set -euo pipefail to the merged validator.
This script now branches across two manifest trees and depends on long sed|grep|jq pipelines. Without nounset and pipefail, missing files or failed pipe stages can propagate partial state and produce the wrong validation result instead of stopping immediately.
Suggested change
#!/bin/bash
+set -euo pipefail
+
#
# This script serves to check and validate the `params.env` file that containsAs per coding guidelines, **/*.sh: "Use 'set -euo pipefail' at script start".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # When KONFLUX=yes, validate the RHOAI manifests; otherwise default to ODH. | |
| if [ "${KONFLUX:-no}" = 'yes' ]; then | |
| _MANIFESTS_VARIANT="rhoai" | |
| # This value needs to be updated everytime we deliberately change number of the | |
| # images we want to have in the `params.env` or `params-latest.env` file. | |
| EXPECTED_COMMIT_NUM_RECORDS=74 | |
| EXPECTED_PARAMS_NUM_RECORDS=74 | |
| else | |
| _MANIFESTS_VARIANT="odh" | |
| # This value needs to be updated everytime we deliberately change number of the | |
| # images we want to have in the `params.env` or `params-latest.env` file. | |
| EXPECTED_COMMIT_NUM_RECORDS=39 | |
| EXPECTED_PARAMS_NUM_RECORDS=33 | |
| fi | |
| COMMIT_LATEST_ENV_PATH="manifests/${_MANIFESTS_VARIANT}/base/commit-latest.env" | |
| COMMIT_ENV_PATH="manifests/${_MANIFESTS_VARIANT}/base/commit.env" | |
| PARAMS_LATEST_ENV_PATH="manifests/${_MANIFESTS_VARIANT}/base/params-latest.env" | |
| PARAMS_ENV_PATH="manifests/${_MANIFESTS_VARIANT}/base/params.env" | |
| #!/bin/bash | |
| set -euo pipefail | |
| # | |
| # This script serves to check and validate the `params.env` file that contains |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ci/check-params-env.sh` around lines 26 - 44, The merged validator script
lacks strict shell options, so failures in file access or any stage of the
sed|grep|jq pipelines can be masked; add "set -euo pipefail" near the top of
ci/check-params-env.sh (immediately after the shebang) to enable exit-on-error,
treat unset variables as errors, and make pipelines fail on any stage; this will
make validation of paths like COMMIT_LATEST_ENV_PATH, PARAMS_ENV_PATH and the
conditional branch on KONFLUX robust and fail fast on missing files or broken
pipeline commands.
jiridanek
force-pushed
the
jd_dedup_check_params_env
branch
from
b85339d to
7dee38b
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
ci/check-params-env.sh (2)
26-44:⚠️ Potential issue | 🟠 MajorAdd strict mode before this variant-selection block.
The merged validator still runs without
set -euo pipefail, so a missing env file or a failedsed|grep|jqstage can leave partial state behind and misreport the real failure.Proposed fix
#!/bin/bash +set -euo pipefail + # # This script serves to check and validate the `params.env` file that contains#!/bin/bash sed -n '1,12p' ci/check-params-env.sh rg -n '^set -euo pipefail$' ci/check-params-env.shAs per coding guidelines,
**/*.sh: "Use 'set -euo pipefail' at script start".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci/check-params-env.sh` around lines 26 - 44, Add Bash strict mode by inserting "set -euo pipefail" at the top of the script before the variant-selection block (i.e., before the KONFLUX check that sets _MANIFESTS_VARIANT, EXPECTED_COMMIT_NUM_RECORDS, etc.). Ensure the statement is executed unconditionally (not inside a conditional) so that missing env files or failed pipelines (sed|grep|jq) cause immediate script exit; if you expect callers to override, keep it as the first active line after the shebang.
794-818:⚠️ Potential issue | 🟠 MajorNormalize
-c9son the repository side too.These new
*-c9sentries still go throughcheck_image_repo_name(), but that helper strips-c9sonly fromimage_variable_filtered. If the Konflux repo basename also ends with-c9s, the equality check fails even when the image is correct.Proposed fix
- repository_name=$(echo "${image_repo##*/}" | sed -E 's/-(ubi9|rhel9)$//') + repository_name=$(echo "${image_repo##*/}" | sed -E 's/-(ubi9|rhel9|c9s)$//')#!/bin/bash sed -n '894,912p' ci/check-params-env.sh rg -n 'c9s' ci/check-params-env.shAs per coding guidelines,
**: "REVIEW PRIORITIES: 3. Bug-prone patterns and error handling gaps".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci/check-params-env.sh` around lines 794 - 818, check_image_repo_name() currently strips the "-c9s" suffix only from image_variable_filtered which causes mismatches when the Konflux repo basename also ends with "-c9s"; update check_image_repo_name() to normalize both sides by removing a trailing "-c9s" from the repo basename (the variable holding the konflux repository name) as well as from image_variable_filtered before comparing, so both values are compared in their normalized form.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@ci/check-params-env.sh`:
- Around line 26-44: Add Bash strict mode by inserting "set -euo pipefail" at
the top of the script before the variant-selection block (i.e., before the
KONFLUX check that sets _MANIFESTS_VARIANT, EXPECTED_COMMIT_NUM_RECORDS, etc.).
Ensure the statement is executed unconditionally (not inside a conditional) so
that missing env files or failed pipelines (sed|grep|jq) cause immediate script
exit; if you expect callers to override, keep it as the first active line after
the shebang.
- Around line 794-818: check_image_repo_name() currently strips the "-c9s"
suffix only from image_variable_filtered which causes mismatches when the
Konflux repo basename also ends with "-c9s"; update check_image_repo_name() to
normalize both sides by removing a trailing "-c9s" from the repo basename (the
variable holding the konflux repository name) as well as from
image_variable_filtered before comparing, so both values are compared in their
normalized form.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Repository UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 8ad4b920-451e-46b2-9a0f-a74d2552eba0
📒 Files selected for processing (3)
.github/workflows/params-env.yamlci/check-params-env-odh.shci/check-params-env.sh
💤 Files with no reviewable changes (1)
- ci/check-params-env-odh.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/params-env.yaml
jiridanek
force-pushed
the
jd_dedup_check_params_env
branch
from
7dee38b to
49fc65d
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 (2)
ci/check-params-env.sh (2)
1108-1118:⚠️ Potential issue | 🟡 MinorIncorrect conditional syntax passes spurious arguments.
Line 1110:
if process_file "${file_}" -eq 0; thenpasses-eqand0as arguments toprocess_file(which ignores them). The conditional still evaluatesprocess_file's exit code, so it "works" by accident, but the syntax is misleading.for file_ in "${PARAMS_ENV_PATH}" "${PARAMS_LATEST_ENV_PATH}"; do echo "Checking file: '${file_}'" - if process_file "${file_}" -eq 0; then + if process_file "${file_}"; then echo "Validation of '${file_}' was successful! Congrats :)" echo "------------------------" else🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci/check-params-env.sh` around lines 1108 - 1118, The if-statement is passing spurious arguments (`-eq 0`) to process_file which is misleading; change the conditional to test the command's exit status directly by calling process_file with the file variable (process_file "${file_}") in the if condition (i.e., use if process_file "${file_}"; then) so the shell evaluates the command's exit code correctly; update references in this loop that iterate over PARAMS_ENV_PATH and PARAMS_LATEST_ENV_PATH and ensure ret_code is still set to 1 on failure.
62-63:⚠️ Potential issue | 🔴 CriticalFix incorrect parameter expansion syntax that prevents function from being called with fewer arguments.
${3:=false}and${4:=false}attempt to assign to positional parameters. Bash errors withcannot assign in this wayif parameters are unset. Use${3:-false}for default value substitution without assignment.Diff
- local allow_value_duplicity="${3:=false}" - local is_params_env="${4:=false}" + local allow_value_duplicity="${3:-false}" + local is_params_env="${4:-false}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci/check-params-env.sh` around lines 62 - 63, The parameter defaulting is using assignment expansion `${3:=false}` and `${4:=false}` which tries to assign to positional parameters and can error; change both to use non-assigning default substitution `${3:-false}` and `${4:-false}` where `allow_value_duplicity` and `is_params_env` are set so the function can be called with fewer args (locate the lines that define `allow_value_duplicity` and `is_params_env` and replace `:=` with `:-`).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@ci/check-params-env.sh`:
- Around line 1108-1118: The if-statement is passing spurious arguments (`-eq
0`) to process_file which is misleading; change the conditional to test the
command's exit status directly by calling process_file with the file variable
(process_file "${file_}") in the if condition (i.e., use if process_file
"${file_}"; then) so the shell evaluates the command's exit code correctly;
update references in this loop that iterate over PARAMS_ENV_PATH and
PARAMS_LATEST_ENV_PATH and ensure ret_code is still set to 1 on failure.
- Around line 62-63: The parameter defaulting is using assignment expansion
`${3:=false}` and `${4:=false}` which tries to assign to positional parameters
and can error; change both to use non-assigning default substitution
`${3:-false}` and `${4:-false}` where `allow_value_duplicity` and
`is_params_env` are set so the function can be called with fewer args (locate
the lines that define `allow_value_duplicity` and `is_params_env` and replace
`:=` with `:-`).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Repository UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 756a5775-0c68-41ed-a9c4-4ee5109c8b94
📒 Files selected for processing (3)
.github/workflows/params-env.yamlci/check-params-env-odh.shci/check-params-env.sh
💤 Files with no reviewable changes (1)
- ci/check-params-env-odh.sh
jiridanek
force-pushed
the
jd_dedup_check_params_env
branch
from
49fc65d to
cc1f06f
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
tests/containers/params_env_validation_test.py (3)
198-201: Hardcoded record counts will break on any image addition/removal.Every time an image is added or removed, both
expected-image-metadata.yamlAND this dict must be updated. Consider deriving expected counts from the YAML by filtering entries wherevariantis invariantslist:metadata = _load_expected_metadata() expected_params = sum(1 for e in metadata.values() if variant in e.get("variants", []))Alternatively, document the maintenance requirement prominently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/containers/params_env_validation_test.py` around lines 198 - 201, The hardcoded expected_counts dict in params_env_validation_test.py will break when images change; replace the static counts by deriving expected params/commit counts from the expected-image-metadata YAML: call _load_expected_metadata(), then compute expected_params as sum(1 for e in metadata.values() if variant in e.get("variants", [])) (and similarly compute expected_commit if needed by filtering on whatever key represents commit entries), and use those computed values instead of the hardcoded expected_counts for the "odh" and "rhoai" branches.
163-163: Docstring incomplete: also checks commit.env uniqueness.Line 164 iterates over
["params", "commit"], but docstring only mentionsparams.env.- """All variable names in params.env + params-latest.env are unique.""" + """All variable names in params/commit .env + *-latest.env are unique."""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/containers/params_env_validation_test.py` at line 163, Update the test docstring to accurately describe what the test checks: it verifies that all variable names across params.env, params-latest.env, and commit.env are unique. Locate the docstring (the triple-quoted string that currently reads "All variable names in params.env + params-latest.env are unique.") and change it to mention params.env, params-latest.env, and commit.env (matching the iteration over ["params", "commit"]) so the docstring reflects the actual checked files.
138-144: Regexn.*is greedy—may over-strip.Pattern
-n.*$matches-notebookif a variable ever ends that way. Current variable names appear safe, but consider tightening ton(-\d+)?$to match only-nor-n-1patterns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/containers/params_env_validation_test.py` around lines 138 - 144, The regex in _strip_version_suffix is too greedy (r'-(\d{4}-\d+|n.*)$') and can over-strip names like '-notebook'; update the pattern used in _strip_version_suffix to match only the intended suffixes (e.g., years like 'YYYY-N' and the specific '-n' or '-n-<digits>' variants) by replacing the 'n.*' branch with a stricter alternative such as matching 'n' optionally followed by a hyphen and digits (e.g., 'n(-\d+)?$') so only '-n' or '-n-1' style suffixes are removed. Ensure the updated regex still handles the year pattern (\d{4}-\d+) and include both alternatives in the same alternation.ci/expected-image-metadata.yaml (1)
1-7: Static analysis: yamllint style warnings are non-blocking.The yamllint indentation warnings (expecting 4-space, found 2-space) are style preferences, not errors. The 2-space indentation is valid YAML and consistent throughout. The missing
---document start is also optional per YAML spec.If the project enforces yamllint rules via CI, add
---at line 1 and either adjust indentation or add a.yamllintconfig to permit 2-space indent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci/expected-image-metadata.yaml` around lines 1 - 7, Add a YAML document start and/or conform to the project's yamllint rules: insert a leading `---` at the top of the ci/expected-image-metadata.yaml file and then either (A) convert the file's 2-space indentation to 4-space indentation to satisfy yamllint expectations, or (B) add a .yamllint config that allows 2-space indentation (configure indentation: {spaces: 2}) so the current formatting is accepted; reference the generated structure used by ci/check-params-env.sh to ensure the content remains unchanged while only adjusting indentation or lint config.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/containers/params_env_validation_test.py`:
- Line 321: The calculation for percent_change using "percent_change = abs(100 *
actual_size // expected_size - 100)" can raise ZeroDivisionError when
expected_size is 0; update the code around that expression (the percent_change
computation) to guard against expected_size being zero (e.g., check
"expected_size" > 0 before computing percent_change and handle the zero case
explicitly by skipping the percent calculation or defining percent_change as 0
or an appropriate sentinel), so the code never performs the division when
expected_size == 0.
- Around line 277-278: The repo extraction currently keeps the tag when no
digest is present because it uses image_url directly; change the logic around
the repo and repo_name assignment (the variables repo and repo_name) to first
strip any digest (split by "@"), then take the last path segment (split by "/")
and then remove any tag suffix (split by ":" and take the part before the colon)
before calling _strip_os_suffix; update the repo/repo_name assignment to reflect
this order so "registry.io/org/repo:tag" yields "repo".
- Around line 298-307: The commit ID lookup is messy: remove the no-op replace
and use a real commit key lookup; compute commit_key = variable + "-commit" and
try file_commit = commit_entries.get(commit_key, "") first, and only if absent
iterate commit_entries items stripping "-commit" from keys
(ck.replace("-commit", "")) to match variable and set file_commit = cv when
found; also delete the unused prior commit_key or ensure the computed commit_key
is actually used and remove the confusing comment.
---
Nitpick comments:
In `@ci/expected-image-metadata.yaml`:
- Around line 1-7: Add a YAML document start and/or conform to the project's
yamllint rules: insert a leading `---` at the top of the
ci/expected-image-metadata.yaml file and then either (A) convert the file's
2-space indentation to 4-space indentation to satisfy yamllint expectations, or
(B) add a .yamllint config that allows 2-space indentation (configure
indentation: {spaces: 2}) so the current formatting is accepted; reference the
generated structure used by ci/check-params-env.sh to ensure the content remains
unchanged while only adjusting indentation or lint config.
In `@tests/containers/params_env_validation_test.py`:
- Around line 198-201: The hardcoded expected_counts dict in
params_env_validation_test.py will break when images change; replace the static
counts by deriving expected params/commit counts from the
expected-image-metadata YAML: call _load_expected_metadata(), then compute
expected_params as sum(1 for e in metadata.values() if variant in
e.get("variants", [])) (and similarly compute expected_commit if needed by
filtering on whatever key represents commit entries), and use those computed
values instead of the hardcoded expected_counts for the "odh" and "rhoai"
branches.
- Line 163: Update the test docstring to accurately describe what the test
checks: it verifies that all variable names across params.env,
params-latest.env, and commit.env are unique. Locate the docstring (the
triple-quoted string that currently reads "All variable names in params.env +
params-latest.env are unique.") and change it to mention params.env,
params-latest.env, and commit.env (matching the iteration over ["params",
"commit"]) so the docstring reflects the actual checked files.
- Around line 138-144: The regex in _strip_version_suffix is too greedy
(r'-(\d{4}-\d+|n.*)$') and can over-strip names like '-notebook'; update the
pattern used in _strip_version_suffix to match only the intended suffixes (e.g.,
years like 'YYYY-N' and the specific '-n' or '-n-<digits>' variants) by
replacing the 'n.*' branch with a stricter alternative such as matching 'n'
optionally followed by a hyphen and digits (e.g., 'n(-\d+)?$') so only '-n' or
'-n-1' style suffixes are removed. Ensure the updated regex still handles the
year pattern (\d{4}-\d+) and include both alternatives in the same alternation.
🪄 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 YAML (base), Central YAML (inherited), Repository UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 0edd453e-25db-48db-8309-66263d59d682
📒 Files selected for processing (2)
ci/expected-image-metadata.yamltests/containers/params_env_validation_test.py
| base = image_url.rsplit("@", 1)[0] | ||
| base = base.rsplit(":", 1)[0] | ||
| platform_raw = _skopeo_inspect_raw(f"{base}@{amd64['digest']}") |
There was a problem hiding this comment.
Edge case: URL with port number will be incorrectly truncated.
If image_url is registry.io:5000/repo@sha256:abc, line 128 rsplit(":", 1)[0] yields registry.io:5000/repo@sha256 (incorrect). Consider anchoring the split to avoid port confusion:
- base = image_url.rsplit("@", 1)[0]
- base = base.rsplit(":", 1)[0]
+ # Remove digest (`@sha256`:...) then tag (:tag), but preserve port
+ base = image_url.rsplit("@", 1)[0]
+ # Only strip tag if there's a / after the last :
+ if "/" not in base.rsplit(":", 1)[-1]:
+ base = base.rsplit(":", 1)[0]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| base = image_url.rsplit("@", 1)[0] | |
| base = base.rsplit(":", 1)[0] | |
| platform_raw = _skopeo_inspect_raw(f"{base}@{amd64['digest']}") | |
| # Remove digest (`@sha256`:...) then tag (:tag), but preserve port | |
| base = image_url.rsplit("@", 1)[0] | |
| # Only strip tag if there's a / after the last : | |
| if "/" not in base.rsplit(":", 1)[-1]: | |
| base = base.rsplit(":", 1)[0] | |
| platform_raw = _skopeo_inspect_raw(f"{base}@{amd64['digest']}") |
| repo = image_url.split("@")[0].split(":")[-1] if "@" in image_url else image_url | ||
| repo_name = _strip_os_suffix(repo.split("/")[-1]) |
There was a problem hiding this comment.
Repo name extraction fails for tag-only URLs (no @).
Line 277: if "@" in image_url else image_url returns the full URL when no digest is present. Subsequent .split("/")[-1] on a full URL like registry.io/org/repo:tag yields repo:tag, not repo.
- repo = image_url.split("@")[0].split(":")[-1] if "@" in image_url else image_url
- repo_name = _strip_os_suffix(repo.split("/")[-1])
+ # Extract repo name: strip digest, then tag, then path
+ repo = image_url.split("@")[0] # remove digest if present
+ repo = repo.rsplit("/", 1)[-1] # last path segment
+ repo_name = repo.split(":")[0] # remove tag
+ repo_name = _strip_os_suffix(repo_name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/containers/params_env_validation_test.py` around lines 277 - 278, The
repo extraction currently keeps the tag when no digest is present because it
uses image_url directly; change the logic around the repo and repo_name
assignment (the variables repo and repo_name) to first strip any digest (split
by "@"), then take the last path segment (split by "/") and then remove any tag
suffix (split by ":" and take the part before the colon) before calling
_strip_os_suffix; update the repo/repo_name assignment to reflect this order so
"registry.io/org/repo:tag" yields "repo".
| commit_key = re.sub(r'-commit', '', variable) # variable without -commit | ||
| # Actually look up: the commit.env key is the variable name itself | ||
| # but commit.env stores: key-commit-suffix=hash | ||
| file_commit = commit_entries.get(variable.replace(variable, variable), "") | ||
| # Find matching key in commit entries by stripping -commit from commit.env keys | ||
| for ck, cv in commit_entries.items(): | ||
| stripped = ck.replace("-commit", "") | ||
| if stripped == variable: | ||
| file_commit = cv | ||
| break |
There was a problem hiding this comment.
Dead code and confusing commit ID lookup logic.
- Line 298:
commit_keyis computed but never used. - Line 301:
variable.replace(variable, variable)is a no-op—always returnsvariable. - The loop logic seems inverted: it strips
-commitfrom commit.env keys to match the params.env variable, but the comment at 297-300 is confusing.
- # commit.env keys have "-commit" inserted
- commit_key = re.sub(r'-commit', '', variable) # variable without -commit
- # Actually look up: the commit.env key is the variable name itself
- # but commit.env stores: key-commit-suffix=hash
- file_commit = commit_entries.get(variable.replace(variable, variable), "")
- # Find matching key in commit entries by stripping -commit from commit.env keys
- for ck, cv in commit_entries.items():
- stripped = ck.replace("-commit", "")
- if stripped == variable:
- file_commit = cv
- break
+ # commit.env keys include "-commit" suffix; strip it to match params.env variable
+ file_commit = ""
+ for ck, cv in commit_entries.items():
+ if ck.replace("-commit", "") == variable:
+ file_commit = cv
+ break🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/containers/params_env_validation_test.py` around lines 298 - 307, The
commit ID lookup is messy: remove the no-op replace and use a real commit key
lookup; compute commit_key = variable + "-commit" and try file_commit =
commit_entries.get(commit_key, "") first, and only if absent iterate
commit_entries items stripping "-commit" from keys (ck.replace("-commit", ""))
to match variable and set file_commit = cv when found; also delete the unused
prior commit_key or ensure the computed commit_key is actually used and remove
the confusing comment.
There was a problem hiding this comment.
@coderabbitai evaluate usefulness of test: Looking at the test, it validates three things:
-
test_params_env_variable_uniqueness — no duplicate keys or values across params.env + params-latest.env (and commit.env). Catches copy-paste errors when adding new image entries.
-
test_params_env_record_count — hardcoded expected counts (ODH: 33 params/39 commits, RHOAI: 74/74). Catches accidentally added or dropped entries.
-
test_params_env_image_metadata — for each image URL in params.env, calls skopeo inspect and checks:
- repo name: the quay.io repo name matches the variable name (catches wrong URL pasted for a variable)
- commit ID: the image's vcs-ref label matches what's in commit.env (catches stale commit.env after image rebuild)
- image name label: matches expected value from YAML (catches wrong image assigned to a variable)
- commitref: for older OpenShift-CI images, the branch label matches expected
- image size: within 10% / 100MB of expected (catches bloat or broken builds with missing layers)
Is it useful? It's a direct port of the 1100-line shell script ci/check-params-env.sh that already runs in CI. The checks themselves are valuable — they catch real mistakes like:
- Wrong image digest pasted for a variable
- commit.env out of sync with actual images
- Unexpected size changes indicating build problems
The weakness is the maintenance burden of expected-image-metadata.yaml — every new release requires updating ~20 entries with new expected names and sizes. The shell script has the same problem (giant case
statement). The record count hardcoding is also fragile.
If you're asking whether this pytest rewrite is worth keeping over the shell script: the structured subtest output is better (you see exactly which image/check failed vs the shell script's wall of text), and
it shares infrastructure with the rest of the test suite. But it's essentially the same checks with the same maintenance cost.
There was a problem hiding this comment.
@jiridanek, here's a frank evaluation of each check's value/cost ratio:
Check 1: test_params_env_variable_uniqueness — Keep as-is
High signal, near-zero maintenance. Purely structural — it will never need updating unless the file format changes. The uniqueness-of-values check (not just keys) is the meaningful part that catches the "wrong digest copy-pasted" class of error before skopeo even runs.
Check 2: test_params_env_record_count — Reconsider the implementation
This is the most fragile check. Hardcoded counts (33/39/74) will fail on every intentional addition, generating noise and friction. Two alternatives worth considering:
- Derive the expected count from
expected-image-metadata.yaml: filter entries by variant, count them. The YAML is the source of truth anyway — the count check would then be "every YAML entry has a corresponding params.env entry and vice versa", which is far more useful than a raw integer. - Replace with a bidirectional membership check: assert that the set of keys in
params.env== the set of keys inexpected-image-metadata.yaml(filtered by variant). This catches both additions and deletions without a hardcoded number.
Check 3: test_params_env_image_metadata — Keep, but the YAML is an improvement worth preserving
The YAML is strictly better than the shell case statement for one reason: it's auto-generatable. The maintenance burden is real, but consider adding a --generate mode or a separate script that scrapes current image labels via skopeo inspect and regenerates the YAML. That converts "manual update 20 entries" into "run script, commit diff, review diff" — the review of the generated diff is where the human judgment actually lives.
The size_mb thresholds drifting over time is unavoidable, but the 10%/100MB tolerance is generous enough that this won't fire on minor layer changes.
On coexistence vs. replacement
The commit message marks this as WIP with known gaps (missing ODH entries, commit-ID lookup logic). If both the shell script and the pytest are running in CI simultaneously, you're paying double maintenance cost. The cleaner path: finish the pytest port and remove the shell script, since the pytest version is objectively superior in debuggability. The @pytest.mark.manifest_validation gate already isolates the skopeo-dependent tests from unit tests, so there's no infrastructure bloat.
Bottom line: The test is useful and worth keeping. The record-count check is the only one worth refactoring — deriving it from the YAML instead of hardcoding eliminates the most common source of spurious failures.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ysok 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 |
Merge check-params-env-odh.sh and check-params-env-rhoai.sh into a single check-params-env.sh that uses the KONFLUX env var to select the manifest variant (ODH by default, RHOAI when KONFLUX=yes). Key changes: - Build system detection: explicitly identify Konflux vs OpenShift-CI images by checking for io.openshift.build.commit.id label, then apply strict validation appropriate for each build system - Konflux: requires vcs-ref, skips commitref (label doesn't exist) - OpenShift-CI: requires both commit.id and commit.ref labels - Always strip OS suffixes (-ubi9, -rhel9, -c9s) in repo name check (harmless for ODH, necessary for RHOAI) - Add ODH-only RStudio py312 entries to the unified case statement Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix value uniqueness check: sed was extracting variable names (LHS)
instead of values (RHS), so duplicate image refs were never detected
- Fix process_file call: remove misleading `-eq 0` args that were
silently ignored (if already tests exit code)
- Remove dead EXPECTED_ADDI_RUNTIME_RECORDS (always 0, no-op addition)
- Remove dead `test -n "${image_name}"` after jq --exit-status already
guarantees non-null
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Rewrite of ci/check-params-env.sh as pytest with structured output. Uses skopeo to validate image labels, commit IDs, repo names, and sizes. Expected metadata extracted from shell script case statement to YAML. Known issues: - Some ODH entries missing from expected-image-metadata.yaml - Repo name URL parsing needs fix for @sha256: refs - Commit ID lookup logic needs correction Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
jiridanek
force-pushed
the
jd_dedup_check_params_env
branch
from
1fa7cc7 to
9ff6b52
Compare
|
New changes are detected. LGTM label has been removed. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ci/check-params-env.sh (1)
60-61:⚠️ Potential issue | 🟠 MajorIncorrect parameter expansion:
:=vs:-.Lines 60-61 use
${3:=false}which attempts assignment to read-only positional parameters. Use:-for default values:- local allow_value_duplicity="${3:=false}" - local is_params_env="${4:=false}" + local allow_value_duplicity="${3:-false}" + local is_params_env="${4:-false}"With
set -uenabled, the current syntax would fail on unset parameters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci/check-params-env.sh` around lines 60 - 61, The parameter expansion for local variables allow_value_duplicity and is_params_env currently uses the assignment operator ${3:=false} and ${4:=false} which tries to assign to positional parameters and will fail with set -u; change both to use the default-value expansion ${3:-false} and ${4:-false} respectively so the script uses fallback values without attempting assignment (update the expansions where local allow_value_duplicity and local is_params_env are defined)..github/workflows/params-env.yaml (1)
53-59:⚠️ Potential issue | 🟡 MinorSecret file may persist on failure before cleanup.
If
git-crypt unlockfails, the key file remains on disk. Use a trap or conditional cleanup.- name: Unlock encrypted secrets with git-crypt run: | echo "${GIT_CRYPT_KEY}" | base64 --decode > ./git-crypt-key - git-crypt unlock ./git-crypt-key - rm ./git-crypt-key + trap 'rm -f ./git-crypt-key' EXIT + git-crypt unlock ./git-crypt-key env: GIT_CRYPT_KEY: ${{ secrets.GIT_CRYPT_KEY }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/params-env.yaml around lines 53 - 59, The step "Unlock encrypted secrets with git-crypt" writes a temporary key to ./git-crypt-key but may leave it on disk if git-crypt unlock fails; update the run block to create a secure temp file (e.g., via mktemp or a variable like git_crypt_key_file), set a trap to always remove that temp file on exit/failure (trap 'rm -f "$git_crypt_key_file"' EXIT) and/or check the exit status of git-crypt unlock before proceeding, then remove the temp file in the cleanup so the key cannot persist; reference the commands `echo "${GIT_CRYPT_KEY}" | base64 --decode > ./git-crypt-key`, `git-crypt unlock ./git-crypt-key`, and the file path `./git-crypt-key` when making these changes.
♻️ Duplicate comments (4)
tests/containers/params_env_validation_test.py (3)
275-281:⚠️ Potential issue | 🟡 MinorRepo name extraction fails for tag-only URLs (no digest).
Line 277: when no
@is present, it returns the full URL. Thenrepo.split("/")[-1]onregistry.io/org/repo:tagyieldsrepo:tag, notrepo.with subtests.test(msg=f"{variable}: repo name"): var_filtered = _strip_os_suffix(_strip_version_suffix(variable)) - repo = image_url.split("@")[0].split(":")[-1] if "@" in image_url else image_url - repo_name = _strip_os_suffix(repo.split("/")[-1]) + # Strip digest, then get last path segment, then remove tag + repo = image_url.split("@")[0] + repo = repo.rsplit("/", 1)[-1] + repo_name = repo.split(":")[0] + repo_name = _strip_os_suffix(repo_name) assert var_filtered == repo_name, (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/containers/params_env_validation_test.py` around lines 275 - 281, The repo extraction logic incorrectly returns "repo:tag" for tag-only image URLs; update the block that computes repo/repo_name (using image_url, repo, repo_name, _strip_os_suffix, _strip_version_suffix) to first take the part before any "@" then take the last path segment and strip any tag by splitting that segment on ":" (or rsplit with maxsplit=1) before passing to _strip_os_suffix; i.e., ensure repo is computed as the last path component without a trailing ":tag" so repo_name becomes the clean repo base name.
127-135:⚠️ Potential issue | 🟡 MinorURL parsing fails for registries with port numbers.
If
image_urlisregistry.io:5000/repo@sha256:abc, line 128rsplit(":", 1)[0]yieldsregistry.io:5000/repo@sha256(incorrect).base = image_url.rsplit("@", 1)[0] - base = base.rsplit(":", 1)[0] + # Only strip tag if there's a / after the last colon (avoid stripping port) + if "/" not in base.rsplit(":", 1)[-1]: + base = base.rsplit(":", 1)[0] platform_raw = _skopeo_inspect_raw(f"{base}@{amd64['digest']}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/containers/params_env_validation_test.py` around lines 127 - 135, The URL parsing incorrectly strips colons in registry host:port by using base = image_url.rsplit(":", 1)[0]; instead, after removing the digest with image_url.rsplit("@", 1)[0] (variable base), only strip a tag from the last path segment, not the whole string: locate the last '/' in base, examine the segment after that, and if that segment contains a ':' remove the tag portion (split that segment on ':' and rejoin with the preceding path); update the code that builds the skopeo inspect target (used before calling _skopeo_inspect_raw) to use this corrected base so registries with ports (e.g., registry.io:5000/repo@sha256:...) are handled correctly.
294-313: 🛠️ Refactor suggestion | 🟠 MajorDead code and no-op in commit ID lookup.
- Line 298:
commit_keyis unused- Line 301:
variable.replace(variable, variable)is a no-op (always returnsvariable)# --- Check commit ID matches commit.env --- if commit_id and "odh-pipeline-runtime-" not in variable: short_commit = commit_id[:7] - # commit.env keys have "-commit" inserted - commit_key = re.sub(r'-commit', '', variable) # variable without -commit - # Actually look up: the commit.env key is the variable name itself - # but commit.env stores: key-commit-suffix=hash - file_commit = commit_entries.get(variable.replace(variable, variable), "") - # Find matching key in commit entries by stripping -commit from commit.env keys + # commit.env keys include "-commit" suffix; strip it to match params.env variable + file_commit = "" for ck, cv in commit_entries.items(): - stripped = ck.replace("-commit", "") - if stripped == variable: + if ck.replace("-commit", "") == variable: file_commit = cv break🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/containers/params_env_validation_test.py` around lines 294 - 313, The code contains dead/no-op operations when resolving the commit value: remove the unused commit_key variable or actually use it, and replace the pointless variable.replace(variable, variable) lookup with a real lookup using the correct key from commit_entries (e.g., compute commit_key by stripping/adding the "-commit" suffix from variable and then use commit_entries.get(commit_key) or iterate commit_entries as the existing loop does); ensure file_commit is assigned from commit_entries using the proper key (commit_key or matched ck) and then compare short_commit to file_commit.ci/check-params-env.sh (1)
1-1:⚠️ Potential issue | 🟠 MajorMissing
set -euo pipefailat script start.As per coding guidelines,
**/*.sh: "Use 'set -euo pipefail' at script start". Without this, failed pipe stages or unset variables propagate silently, potentially producing incorrect validation results.#!/bin/bash +set -euo pipefail # # This script serves to check and validate the `params.env` file that contains🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci/check-params-env.sh` at line 1, Add strict error handling to the script by inserting "set -euo pipefail" immediately after the existing shebang ("#!/bin/bash") at the top of the file so the script exits on errors, treats unset variables as errors, and fails on pipeline errors; ensure this change is added to the top of the script that contains the shebang so subsequent checks and variable usage (including any references to environment params) won't silently fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ci/expected-image-metadata.yaml`:
- Around line 1-14: The YAML file ci/expected-image-metadata.yaml is missing a
document start marker and has inconsistent indentation; add a leading document
marker ("---") at the top of the file and normalize indentation to 4 spaces for
mappings and list items so entries under the key
odh-workbench-jupyter-minimal-cpu-py311-ubi9-n (name, commitref, build_name,
size_mb, and the variants list) are indented by 4 spaces and list items (e.g., -
rhoai) are aligned with 4-space indentation, or alternatively update your
yamllint config to accept 2-space indentation if you prefer that style.
---
Outside diff comments:
In @.github/workflows/params-env.yaml:
- Around line 53-59: The step "Unlock encrypted secrets with git-crypt" writes a
temporary key to ./git-crypt-key but may leave it on disk if git-crypt unlock
fails; update the run block to create a secure temp file (e.g., via mktemp or a
variable like git_crypt_key_file), set a trap to always remove that temp file on
exit/failure (trap 'rm -f "$git_crypt_key_file"' EXIT) and/or check the exit
status of git-crypt unlock before proceeding, then remove the temp file in the
cleanup so the key cannot persist; reference the commands `echo
"${GIT_CRYPT_KEY}" | base64 --decode > ./git-crypt-key`, `git-crypt unlock
./git-crypt-key`, and the file path `./git-crypt-key` when making these changes.
In `@ci/check-params-env.sh`:
- Around line 60-61: The parameter expansion for local variables
allow_value_duplicity and is_params_env currently uses the assignment operator
${3:=false} and ${4:=false} which tries to assign to positional parameters and
will fail with set -u; change both to use the default-value expansion
${3:-false} and ${4:-false} respectively so the script uses fallback values
without attempting assignment (update the expansions where local
allow_value_duplicity and local is_params_env are defined).
---
Duplicate comments:
In `@ci/check-params-env.sh`:
- Line 1: Add strict error handling to the script by inserting "set -euo
pipefail" immediately after the existing shebang ("#!/bin/bash") at the top of
the file so the script exits on errors, treats unset variables as errors, and
fails on pipeline errors; ensure this change is added to the top of the script
that contains the shebang so subsequent checks and variable usage (including any
references to environment params) won't silently fail.
In `@tests/containers/params_env_validation_test.py`:
- Around line 275-281: The repo extraction logic incorrectly returns "repo:tag"
for tag-only image URLs; update the block that computes repo/repo_name (using
image_url, repo, repo_name, _strip_os_suffix, _strip_version_suffix) to first
take the part before any "@" then take the last path segment and strip any tag
by splitting that segment on ":" (or rsplit with maxsplit=1) before passing to
_strip_os_suffix; i.e., ensure repo is computed as the last path component
without a trailing ":tag" so repo_name becomes the clean repo base name.
- Around line 127-135: The URL parsing incorrectly strips colons in registry
host:port by using base = image_url.rsplit(":", 1)[0]; instead, after removing
the digest with image_url.rsplit("@", 1)[0] (variable base), only strip a tag
from the last path segment, not the whole string: locate the last '/' in base,
examine the segment after that, and if that segment contains a ':' remove the
tag portion (split that segment on ':' and rejoin with the preceding path);
update the code that builds the skopeo inspect target (used before calling
_skopeo_inspect_raw) to use this corrected base so registries with ports (e.g.,
registry.io:5000/repo@sha256:...) are handled correctly.
- Around line 294-313: The code contains dead/no-op operations when resolving
the commit value: remove the unused commit_key variable or actually use it, and
replace the pointless variable.replace(variable, variable) lookup with a real
lookup using the correct key from commit_entries (e.g., compute commit_key by
stripping/adding the "-commit" suffix from variable and then use
commit_entries.get(commit_key) or iterate commit_entries as the existing loop
does); ensure file_commit is assigned from commit_entries using the proper key
(commit_key or matched ck) and then compare short_commit to file_commit.
🪄 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 YAML (base), Central YAML (inherited), Repository UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 9b2c26be-7afa-4f70-9c63-3bde1db86294
📒 Files selected for processing (5)
.github/workflows/params-env.yamlci/check-params-env-odh.shci/check-params-env.shci/expected-image-metadata.yamltests/containers/params_env_validation_test.py
💤 Files with no reviewable changes (1)
- ci/check-params-env-odh.sh
| # Expected image metadata for params.env validation. | ||
| # Generated from ci/check-params-env.sh case statement. | ||
| # Each entry maps a params.env variable name to expected image properties. | ||
| # | ||
| # size_mb thresholds: 10% or 100MB max change from expected. | ||
| # When name/size_mb is a dict, values differ between ODH and RHOAI. | ||
|
|
||
| odh-workbench-jupyter-minimal-cpu-py311-ubi9-n: | ||
| name: odh-notebook-jupyter-minimal-ubi9-python-3.11 | ||
| commitref: main | ||
| build_name: konflux | ||
| size_mb: 1219 | ||
| variants: | ||
| - rhoai |
There was a problem hiding this comment.
Fix YAML formatting issues flagged by static analysis.
Static analysis reports:
- Missing document start marker
---at line 1 - Wrong indentation: expected 4 spaces, found 2 (lines 14, 21, 28, etc.)
Suggested fix for document start and consistent indentation
+---
# Expected image metadata for params.env validation.
# Generated from ci/check-params-env.sh case statement.Then either configure yamllint to accept 2-space indentation or update all list items to use 4 spaces.
🧰 Tools
🪛 GitHub Check: code-static-analysis
[failure] 14-14:
14:3 [indentation] wrong indentation: expected 4 but found 2
[warning] 8-8:
8:1 [document-start] missing document start "---"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ci/expected-image-metadata.yaml` around lines 1 - 14, The YAML file
ci/expected-image-metadata.yaml is missing a document start marker and has
inconsistent indentation; add a leading document marker ("---") at the top of
the file and normalize indentation to 4 spaces for mappings and list items so
entries under the key odh-workbench-jupyter-minimal-cpu-py311-ubi9-n (name,
commitref, build_name, size_mb, and the variants list) are indented by 4 spaces
and list items (e.g., - rhoai) are aligned with 4-space indentation, or
alternatively update your yamllint config to accept 2-space indentation if you
prefer that style.
3 participants
Summary
Merge
ci/check-params-env-odh.sh(653 lines) andci/check-params-env-rhoai.sh(923 lines) into a singleci/check-params-env.sh.bash ci/check-params-env.sh— validates ODH manifests (default)KONFLUX=yes bash ci/check-params-env.sh— validates RHOAI manifestsKey improvements
Build system detection: Explicitly detects Konflux vs OpenShift-CI by checking for
io.openshift.build.commit.idlabel, then applies strict validation per build system.Repo name filtering: Always strips OS suffixes (
-ubi9,-rhel9,-c9s) — harmless for ODH, necessary for RHOAI.Evaluation
Looking at the test, it validates three things:
test_params_env_variable_uniqueness — no duplicate keys or values across params.env + params-latest.env (and commit.env). Catches copy-paste errors when adding new image entries.
test_params_env_record_count — hardcoded expected counts (ODH: 33 params/39 commits, RHOAI: 74/74). Catches accidentally added or dropped entries.
test_params_env_image_metadata — for each image URL in params.env, calls skopeo inspect and checks:
Is it useful? It's a direct port of the 1100-line shell script ci/check-params-env.sh that already runs in CI. The checks themselves are valuable — they catch real mistakes like:
The weakness is the maintenance burden of expected-image-metadata.yaml — every new release requires updating ~20 entries with new expected names and sizes. The shell script has the same problem (giant case
statement). The record count hardcoding is also fragile.
If you're asking whether this pytest rewrite is worth keeping over the shell script: the structured subtest output is better (you see exactly which image/check failed vs the shell script's wall of text), and
it shares infrastructure with the rest of the test suite. But it's essentially the same checks with the same maintenance cost.
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit