MCO-2411: Add AWS marketplace AMI band check target - #6365
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@djoshy: This pull request references MCO-2411 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. |
WalkthroughThe change adds a Marketplace skew-checking CLI, GitHub and AWS data retrieval, report formats, and shared Marketplace AMI helpers. Boot image controllers now use the shared parsing and comparison logic. ChangesMarketplace skew validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant GitHub
participant AWS
participant Report
CLI->>GitHub: retrieve historical skew limits and installer metadata
GitHub-->>CLI: source revisions and release ceilings
CLI->>AWS: query Marketplace AMIs
AWS-->>CLI: AMI metadata
CLI->>Report: write table or JSON results
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: djoshy 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 |
openshift-ci
Bot
added
the
approved
djoshy
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: 4
🧹 Nitpick comments (2)
devex/cmd/check-aws-marketplace-skew/github.go (2)
47-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting shared request/response handling.
fetchRawGitHubFileandfetchGitHubCommitsPageboth build a request, callhttp.DefaultClient.Do, check status codes, and read the body with nearly identical error wrapping. Extracting a smalldoGitHubRequest(ctx, url, headers) ([]byte, int, error)helper would remove this duplication and centralize the 404/500 handling in one place, which also makes theRefNotFoundErrorfix above easier to apply consistently.Also applies to: 100-139
🤖 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 `@devex/cmd/check-aws-marketplace-skew/github.go` around lines 47 - 73, Extract the duplicated request execution, response-body reading, and error wrapping from fetchRawGitHubFile and fetchGitHubCommitsPage into a shared doGitHubRequest(ctx, url, headers) helper returning the response body and status code. Update both callers to use this helper while preserving their existing URL-specific behavior, including consistent RefNotFoundError handling and non-success status reporting.
48-48: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueEscape URL path segments before interpolation.
fmt.Sprintf(githubRawURLTemplate, owner, repo, ref, path)(Line 48) andfmt.Sprintf(githubCommitsAPITemplate, ...)(Line 101) interpolaterefandpathdirectly into the URL withouturl.PathEscape/url.QueryEscape. Current call sites use safe values, but a branch name or path containing characters like#,?, or a space would break the request silently or hit the wrong resource. Escaping these segments is a small, defensive change.Also applies to: 101-101
🤖 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 `@devex/cmd/check-aws-marketplace-skew/github.go` at line 48, Update the URL construction around the raw-content request and the commits API request to escape interpolated ref and path segments before applying githubRawURLTemplate and githubCommitsAPITemplate. Use path-segment escaping appropriate for URL paths, preserving the existing owner/repo structure and request behavior for ordinary values.
🤖 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 `@devex/cmd/check-aws-marketplace-skew/github_test.go`:
- Around line 96-116: Replace require.NoError with assert.NoError for w.Write
errors in the handlers of TestGithubCommitsForPath_paginatesUntilShortPage,
TestGithubCommitsForPath_capsAtMaxPages, and
TestGithubCommitsForPath_branchNotFound, keeping the existing error checks
otherwise unchanged.
In `@devex/cmd/check-aws-marketplace-skew/github.go`:
- Around line 35-43: The RefNotFoundError message incorrectly attributes every
raw GitHub 404 to a missing branch. Update fetchRawGitHubFile to return a
neutral error that includes the requested path, while preserving distinct
handling for the commits endpoint where a missing path yields an empty result;
adjust RefNotFoundError only if needed to support this behavior.
In `@devex/cmd/check-aws-marketplace-skew/main.go`:
- Around line 25-30: Update the root command execution flow around rootCmd and
its RunE handler to create a signal-aware context with an appropriate timeout
before invoking run. Pass that derived context instead of context.Background(),
ensuring cancellation propagates to AWS subprocesses and GitHub requests and the
deadline is released when execution completes.
In `@devex/cmd/check-aws-marketplace-skew/report.go`:
- Around line 28-50: Update Report.WriteTable to return errors and check every
fmt.Fprintf, fmt.Fprintln, and tabwriter.Flush call, returning the first failure
instead of discarding it. At the caller in run, propagate the WriteTable error
so main.go returns failure when output writing is incomplete.
---
Nitpick comments:
In `@devex/cmd/check-aws-marketplace-skew/github.go`:
- Around line 47-73: Extract the duplicated request execution, response-body
reading, and error wrapping from fetchRawGitHubFile and fetchGitHubCommitsPage
into a shared doGitHubRequest(ctx, url, headers) helper returning the response
body and status code. Update both callers to use this helper while preserving
their existing URL-specific behavior, including consistent RefNotFoundError
handling and non-success status reporting.
- Line 48: Update the URL construction around the raw-content request and the
commits API request to escape interpolated ref and path segments before applying
githubRawURLTemplate and githubCommitsAPITemplate. Use path-segment escaping
appropriate for URL paths, preserving the existing owner/repo structure and
request behavior for ordinary values.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: cc243c8a-67ca-4f18-94c2-c098b1880b7c
📒 Files selected for processing (19)
Makefiledevex/cmd/check-aws-marketplace-skew/README.mddevex/cmd/check-aws-marketplace-skew/github.godevex/cmd/check-aws-marketplace-skew/github_test.godevex/cmd/check-aws-marketplace-skew/installerceiling.godevex/cmd/check-aws-marketplace-skew/installerceiling_test.godevex/cmd/check-aws-marketplace-skew/main.godevex/cmd/check-aws-marketplace-skew/marketplace.godevex/cmd/check-aws-marketplace-skew/marketplace_test.godevex/cmd/check-aws-marketplace-skew/products.godevex/cmd/check-aws-marketplace-skew/report.godevex/cmd/check-aws-marketplace-skew/skewlimit.godevex/cmd/check-aws-marketplace-skew/skewlimit_test.gopkg/controller/bootimage/aws_helpers.gopkg/controller/bootimage/aws_helpers_test.gopkg/controller/bootimage/marketplace/marketplace.gopkg/controller/bootimage/marketplace/marketplace_test.gopkg/controller/bootimage/ms_helpers.gopkg/controller/bootimage/platform_helpers.go
💤 Files with no reviewable changes (1)
- pkg/controller/bootimage/aws_helpers_test.go
| withFakeGitHubAPI(t, func(w http.ResponseWriter, r *http.Request) { | ||
| requestCount++ | ||
| page, _ := strconv.Atoi(r.URL.Query().Get("page")) | ||
| perPage, _ := strconv.Atoi(r.URL.Query().Get("per_page")) | ||
| require.Equal(t, githubCommitsPerPage, perPage) | ||
|
|
||
| start := (page - 1) * perPage | ||
| end := start + perPage | ||
| if start > total { | ||
| start = total | ||
| } | ||
| if end > total { | ||
| end = total | ||
| } | ||
| shas := make([]string, 0, end-start) | ||
| for i := start; i < end; i++ { | ||
| shas = append(shas, fmt.Sprintf("sha-%d", i)) | ||
| } | ||
| _, err := w.Write(fakeCommitsJSON(t, shas)) | ||
| require.NoError(t, err) | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not call require.NoError inside an httptest handler goroutine.
w.Write errors are checked with require.NoError(t, err) inside the server handlers of TestGithubCommitsForPath_paginatesUntilShortPage (Line 114-115), TestGithubCommitsForPath_capsAtMaxPages (Line 138-139), and TestGithubCommitsForPath_branchNotFound (Line 151-152). These handlers run on a goroutine managed by net/http, not the test's own goroutine. require.NoError calls t.FailNow(), and the Go testing package requires FailNow to be called only from the goroutine running the test. Calling it here does not reliably fail the test; it can instead truncate the in-flight response and produce a flaky or hanging test run.
Use assert.NoError(t, err) in the handler instead, since it does not call FailNow, and it is safe to call from any goroutine.
🛠️ Proposed fix
- _, err := w.Write(fakeCommitsJSON(t, shas))
- require.NoError(t, err)
+ _, err := w.Write(fakeCommitsJSON(t, shas))
+ assert.NoError(t, err)Apply the same change to the other two handlers.
Also applies to: 128-140, 149-153
🤖 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 `@devex/cmd/check-aws-marketplace-skew/github_test.go` around lines 96 - 116,
Replace require.NoError with assert.NoError for w.Write errors in the handlers
of TestGithubCommitsForPath_paginatesUntilShortPage,
TestGithubCommitsForPath_capsAtMaxPages, and
TestGithubCommitsForPath_branchNotFound, keeping the existing error checks
otherwise unchanged.
| // RefNotFoundError indicates a GitHub repo has no branch/ref by this name (HTTP 404). Kept as a | ||
| // distinct type mainly for a clearer error message; nothing currently type-switches on it. | ||
| type RefNotFoundError struct { | ||
| Owner, Repo, Ref, URL string | ||
| } | ||
|
|
||
| func (e *RefNotFoundError) Error() string { | ||
| return fmt.Sprintf("%s/%s has no branch %q (%s returned 404)", e.Owner, e.Repo, e.Ref, e.URL) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files '*github.go' '*installerceiling.go' '*skewlimit.go' '*main.go'
printf '%s\n' '--- github.go outline ---'
f=$(git ls-files '*devex/cmd/check-aws-marketplace-skew/github.go' | head -n1)
ast-grep outline "$f"
printf '%s\n' '--- relevant source ---'
cat -n "$f" | sed -n '1,180p'
printf '%s\n' '--- downstream references ---'
for p in $(git ls-files '*installerceiling.go' '*skewlimit.go' '*main.go'); do
echo "### $p"
rg -n -C 4 'installerStreamPath|skewLimitsPath|fetchRawGitHubFile|fetchGitHubCommitsPage|RefNotFoundError|context\.WithTimeout|http\.Client' "$p" || true
doneRepository: openshift/machine-config-operator
Length of output: 17086
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import json
import urllib.request
import urllib.error
cases = [
("raw-valid-ref-missing-path",
"https://raw.githubusercontent.com/openshift/installer/main/data/data/coreos/does-not-exist.json"),
("raw-missing-ref",
"https://raw.githubusercontent.com/openshift/installer/this-ref-does-not-exist/data/data/coreos/coreos-rhel-10.json"),
("api-valid-ref-missing-path",
"https://api.github.com/repos/openshift/machine-config-operator/commits?sha=main&path=does-not-exist.go&per_page=1&page=1"),
("api-missing-ref",
"https://api.github.com/repos/openshift/machine-config-operator/commits?sha=this-ref-does-not-exist&path=pkg/controller/common/constants.go&per_page=1&page=1"),
]
for name, url in cases:
req = urllib.request.Request(url, headers={"Accept": "application/vnd.github+json", "User-Agent": "review-probe"})
try:
with urllib.request.urlopen(req, timeout=15) as response:
body = response.read(500).decode("utf-8", "replace")
print(name, response.status, body[:200].replace("\n", " "))
except urllib.error.HTTPError as e:
body = e.read(500).decode("utf-8", "replace")
print(name, e.code, body[:200].replace("\n", " "))
except Exception as e:
print(name, type(e).__name__, str(e))
PY
printf '%s\n' '--- local error-type consumers and tests ---'
rg -n -C 3 'RefNotFoundError|has no branch|installerStreamPath|coreos-rhel-(9|10)' devex/cmd/check-aws-marketplace-skew
printf '%s\n' '--- main context setup ---'
cat -n devex/cmd/check-aws-marketplace-skew/main.go | sed -n '1,180p'Repository: openshift/machine-config-operator
Length of output: 10851
🏁 Script executed:
#!/bin/bash
set -eu
probe() {
name=$1
url=$2
result=$(curl -k -sS -L -o /tmp/review-probe-body -w '%{http_code}' \
-H 'Accept: application/vnd.github+json' -A 'review-probe' "$url" || true)
printf '%s %s ' "$name" "$result"
head -c 180 /tmp/review-probe-body | tr '\n' ' '
printf '\n'
}
probe raw-valid-ref-missing-path \
'https://raw.githubusercontent.com/openshift/installer/main/data/data/coreos/does-not-exist.json'
probe raw-missing-ref \
'https://raw.githubusercontent.com/openshift/installer/this-ref-does-not-exist/data/data/coreos/coreos-rhel-10.json'
probe api-valid-ref-missing-path \
'https://api.github.com/repos/openshift/machine-config-operator/commits?sha=main&path=does-not-exist.go&per_page=1&page=1'
probe api-missing-ref \
'https://api.github.com/repos/openshift/machine-config-operator/commits?sha=this-ref-does-not-exist&path=pkg/controller/common/constants.go&per_page=1&page=1'
printf '%s\n' '--- installer branch/path probes ---'
probe installer-release-4-18-rhel10 \
'https://raw.githubusercontent.com/openshift/installer/release-4.18/data/data/coreos/coreos-rhel-10.json'
probe installer-release-4-17-rhel10 \
'https://raw.githubusercontent.com/openshift/installer/release-4.17/data/data/coreos/coreos-rhel-10.json'
probe installer-release-4-17-rhel9 \
'https://raw.githubusercontent.com/openshift/installer/release-4.17/data/data/coreos/coreos-rhel-9.json'Repository: openshift/machine-config-operator
Length of output: 608
Use a neutral error for raw GitHub 404 responses. A valid ref with a missing path also returns 404, so fetchRawGitHubFile must not report that the branch is absent. Include path in the error and use neutral wording. Keep the commits endpoint handling separate because a missing path there returns an empty result.
🤖 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 `@devex/cmd/check-aws-marketplace-skew/github.go` around lines 35 - 43, The
RefNotFoundError message incorrectly attributes every raw GitHub 404 to a
missing branch. Update fetchRawGitHubFile to return a neutral error that
includes the requested path, while preserving distinct handling for the commits
endpoint where a missing path yields an empty result; adjust RefNotFoundError
only if needed to support this behavior.
| rootCmd := &cobra.Command{ | ||
| Use: "check-aws-marketplace-skew", | ||
| Short: "Checks published AWS Marketplace RHCOS AMIs against the MCO boot-image skew band.", | ||
| RunE: func(_ *cobra.Command, _ []string) error { | ||
| return run(context.Background(), region, profile, branch, jsonOut) | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate file ---'
git ls-files 'devex/cmd/check-aws-marketplace-skew/main.go'
wc -l devex/cmd/check-aws-marketplace-skew/main.go
sed -n '1,140p' devex/cmd/check-aws-marketplace-skew/main.go
printf '%s\n' '--- context and external-call usage ---'
rg -n -C 3 'context\.|run\(|http\.|exec\.|aws|github|Marketplace|Timeout|WithTimeout|WithCancel' devex/cmd/check-aws-marketplace-skewRepository: openshift/machine-config-operator
Length of output: 47620
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- dependency versions ---'
rg -n 'spf13/cobra|component-base' go.mod go.sum
printf '%s\n' '--- available dependency source ---'
gopath="$(go env GOPATH 2>/dev/null || true)"
if [ -n "$gopath" ] && [ -d "$gopath/pkg/mod" ]; then
rg -n -g '*.go' 'func \(.*\) Context\(\)|func \(.*\) Execute\(|func Run\(.*\*cobra\.Command' \
"$gopath/pkg/mod/github.com/spf13" \
"$gopath/pkg/mod/k8s.io" 2>/dev/null | head -80 || true
else
printf '%s\n' 'Go module cache is unavailable'
fi
printf '%s\n' '--- structural verifier for the reviewed path ---'
python3 - <<'PY'
from pathlib import Path
import re
root = Path("devex/cmd/check-aws-marketplace-skew")
main = (root / "main.go").read_text()
marketplace = (root / "marketplace.go").read_text()
github = (root / "github.go").read_text()
run_e = re.search(r'RunE:\s*func\(([^)]*)\)\s*error\s*\{(?P<body>.*?)\n\t\t\}', main, re.S)
assert run_e, "RunE was not found"
body = run_e.group("body")
print("RunE parameter list:", run_e.group(1).strip())
print("RunE invokes context.Background():", "context.Background()" in body)
print("RunE invokes cmd.Context():", bool(re.search(r'\b\w+\.Context\(\)', body)))
print("run receives the context:", bool(re.search(r'\brun\(\s*context\.Background\(\)', body)))
print("AWS call uses exec.CommandContext:", "exec.CommandContext(ctx" in marketplace)
print("GitHub calls bind requests to context:",
"http.NewRequestWithContext(ctx" in github)
print("HTTP client has an explicit timeout in reviewed files:",
bool(re.search(r'\b(?:Timeout|Client\s*\{)[^\\n]*', github)))
PYRepository: openshift/machine-config-operator
Length of output: 1547
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Cobra context behavior ---'
sed -n '255,280p' /home/jailuser/go/pkg/mod/github.com/spf13/cobra@v1.10.2/command.go
sed -n '1060,1085p' /home/jailuser/go/pkg/mod/github.com/spf13/cobra@v1.10.2/command.go
printf '%s\n' '--- component-base CLI entry point ---'
rg -n -C 6 'func Run\(' /home/jailuser/go/pkg/mod/github.com/openshift/kubernetes/staging/src/k8s.io/component-base@*/cli 2>/dev/null || trueRepository: openshift/machine-config-operator
Length of output: 4679
🏁 Script executed:
#!/bin/bash
set -eu
sed -n '43,90p' /home/jailuser/go/pkg/mod/github.com/openshift/kubernetes/staging/src/k8s.io/component-base@v0.0.0-20260716191900-78acd4d422b2/cli/run.goRepository: openshift/machine-config-operator
Length of output: 1864
🏁 Script executed:
#!/bin/bash
set -eu
sed -n '90,150p' /home/jailuser/go/pkg/mod/github.com/openshift/kubernetes/staging/src/k8s.io/component-base@v0.0.0-20260716191900-78acd4d422b2/cli/run.goRepository: openshift/machine-config-operator
Length of output: 2000
Add cancellation and a deadline for external calls.
cli.Run calls cmd.Execute(), so cmd.Context() defaults to context.Background(). The AWS subprocess and GitHub requests use this context, and the HTTP client has no separate timeout. Use a signal-aware context with context.WithTimeout before calling run.
🤖 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 `@devex/cmd/check-aws-marketplace-skew/main.go` around lines 25 - 30, Update
the root command execution flow around rootCmd and its RunE handler to create a
signal-aware context with an appropriate timeout before invoking run. Pass that
derived context instead of context.Background(), ensuring cancellation
propagates to AWS subprocesses and GitHub requests and the deadline is released
when execution completes.
Source: Path instructions
| func (r Report) WriteTable(w io.Writer) { | ||
| fmt.Fprintf(w, "Skew-limit floor: RHCOS %s (OCP %s)\n", r.Floor.RHCOS, r.Floor.OCP) | ||
| for arch, token := range r.Ceiling { | ||
| fmt.Fprintf(w, "Installer ceiling (%s): %s\n", arch, token) | ||
| } | ||
| fmt.Fprintln(w) | ||
|
|
||
| tw := tabwriter.NewWriter(w, 0, 0, 2, ' ', 0) | ||
| fmt.Fprintln(tw, "PRODUCT\tPRODUCT ID\tRESULT\tMATCHED AMI\tDETAIL") | ||
| for _, res := range r.Results { | ||
| result := "PASS" | ||
| if !res.Pass { | ||
| result = "FAIL" | ||
| } | ||
| matched := "" | ||
| detail := res.Reason | ||
| if res.MatchedAMI != nil { | ||
| matched = res.MatchedAMI.ImageID | ||
| detail = res.MatchedAMI.Version | ||
| } | ||
| fmt.Fprintf(tw, "%s\t%s\t%s\t%s\t%s\n", res.ProductName, res.ProductID, result, matched, detail) | ||
| } | ||
| tw.Flush() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
report_file="$(fd -t f -p 'report.go' | grep -F 'devex/cmd/check-aws-marketplace-skew/report.go' | head -n 1)"
main_file="$(fd -t f -p 'main.go' | grep -F 'devex/cmd/check-aws-marketplace-skew/main.go' | head -n 1)"
printf '%s\n' "REPORT=$report_file" "MAIN=$main_file"
cat -n "$report_file" | sed -n '1,90p'
printf '\n--- main.go relevant section ---\n'
cat -n "$main_file" | sed -n '45,80p'
printf '\n--- usages of WriteTable ---\n'
rg -n -C 3 'WriteTable\s*\(' --glob '*.go' .Repository: openshift/machine-config-operator
Length of output: 4469
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
report = Path("devex/cmd/check-aws-marketplace-skew/report.go").read_text()
main = Path("devex/cmd/check-aws-marketplace-skew/main.go").read_text()
body = re.search(
r'func \(r Report\) WriteTable\(w io\.Writer\) \{(.*?)\n\}',
report,
re.S,
)
assert body, "WriteTable was not found"
body = body.group(1)
calls = re.findall(r'\b(fmt\.Fprintf|fmt\.Fprintln|tw\.Flush)\s*\(', body)
print("WriteTable calls:", calls)
print("WriteTable returns error:", bool(re.search(r'func \(r Report\) WriteTable\(w io\.Writer\)\s+error', report)))
print("main checks WriteTable error:", bool(re.search(r'if\s+err\s*:=\s*report\.WriteTable\(os\.Stdout\)', main)))
assert calls.count("fmt.Fprintf") == 2
assert calls.count("fmt.Fprintln") == 2
assert calls.count("tw.Flush") == 1
assert "func (r Report) WriteTable(w io.Writer) error" not in report
assert "report.WriteTable(os.Stdout)" in main
PY
printf '\n--- command error handling ---\n'
cat -n devex/cmd/check-aws-marketplace-skew/main.go | sed -n '1,35p'
rg -n -C 3 'run\(|os\.Exit|log\.Fatal|fmt\.Fprint|Execute' devex/cmd/check-aws-marketplace-skew --glob '*.go'Repository: openshift/machine-config-operator
Length of output: 436
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
report = Path("devex/cmd/check-aws-marketplace-skew/report.go").read_text()
main = Path("devex/cmd/check-aws-marketplace-skew/main.go").read_text()
body = re.search(
r'func \(r Report\) WriteTable\(w io\.Writer\) \{(.*?)\n\}',
report,
re.S,
).group(1)
calls = re.findall(r'\b(fmt\.Fprintf|fmt\.Fprintln|tw\.Flush)\s*\(', body)
print("WriteTable calls:", calls)
print("WriteTable returns error:", bool(re.search(
r'func \(r Report\) WriteTable\(w io\.Writer\)\s+error', report)))
print("main checks WriteTable error:", bool(re.search(
r'if\s+err\s*:=\s*report\.WriteTable\(os\.Stdout\)', main)))
assert calls.count("fmt.Fprintf") == 3
assert calls.count("fmt.Fprintln") == 2
assert calls.count("tw.Flush") == 1
assert "func (r Report) WriteTable(w io.Writer) error" not in report
assert "report.WriteTable(os.Stdout)" in main
print("source invariant: all table-output errors are currently discarded")
PY
printf '\n--- command error handling ---\n'
cat -n devex/cmd/check-aws-marketplace-skew/main.go | sed -n '1,35p'
rg -n -C 3 'run\(|os\.Exit|log\.Fatal|fmt\.Fprint|Execute' \
devex/cmd/check-aws-marketplace-skew --glob '*.go' || trueRepository: openshift/machine-config-operator
Length of output: 5436
Propagate table-write errors.
WriteTable discards errors from its fmt.Fprintf, fmt.Fprintln, and tw.Flush calls. If os.Stdout fails, run can return success after incomplete output. Return an error from WriteTable and propagate it at main.go:66.
🤖 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 `@devex/cmd/check-aws-marketplace-skew/report.go` around lines 28 - 50, Update
Report.WriteTable to return errors and check every fmt.Fprintf, fmt.Fprintln,
and tabwriter.Flush call, returning the first failure instead of discarding it.
At the caller in run, propagate the WriteTable error so main.go returns failure
when output writing is incomplete.
Source: Path instructions
|
@djoshy: 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. |
2 participants
- What I did
Marketplace-published RHCOS boot images can drift from what MCO supports without any alerting today. This tool will help in alerting the MCO team when the drift is larger than expected. This is to be run manually by an engineer or an AI agent and not designed to be run as a Prow periodic.
This PR does the following:
pkg/controller/bootimage/marketplacefromaws_helpers.go(shared by the controller and the tool).devex/cmd/check-aws-marketplace-skew(make check-aws-marketplace-skew), checks published Marketplace AMIs per product code against MCO's boot-image skew band, fetching both bounds live from GitHub (no local checkout needed).- How to verify it
AWS marketplace tests should continue to pass, the refactor should have no effect on the functionality of this feature.
When the new target is run locally(assuming AWS and GitHub tokens have been set up), the output should look like so:
The failure above for ARM64 variants is expected: no AMIs with RHEL aligned versions have been published for them(as of writing), and as a result they are not suitable candidates for the band check - the MCO will not use them to perform boot image updates.
Summary by CodeRabbit
New Features
Documentation
Refactor