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
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 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
93c54c9 to
3f1c874
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@devex/cmd/check-aws-marketplace-skew/github.go`:
- Around line 162-165: Validate each raw commit’s SHA and committer date in the
loop that builds commits before appending a ghCommit. If either field is empty
or missing, return an error instead of emitting a zero-value or incomplete
record; preserve the existing conversion for valid records.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a7bedfaa-cc57-4e7b-b93a-d0da49fcde18
📒 Files selected for processing (5)
devex/cmd/check-aws-marketplace-skew/github.godevex/cmd/check-aws-marketplace-skew/github_test.godevex/cmd/check-aws-marketplace-skew/main.godevex/cmd/check-aws-marketplace-skew/marketplace.godevex/cmd/check-aws-marketplace-skew/report.go
🚧 Files skipped from review as they are similar to previous changes (3)
- devex/cmd/check-aws-marketplace-skew/main.go
- devex/cmd/check-aws-marketplace-skew/github_test.go
- devex/cmd/check-aws-marketplace-skew/marketplace.go
|
/retest-required |
1 similar comment
|
/retest-required |
|
@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