clusterimageset-updater: mock release API in integration test - #5323
clusterimageset-updater: mock release API in integration test#5323psalajova wants to merge 4 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe updater accepts an optional release service URL and passes it through prerelease resolution. Candidate and prerelease endpoint helpers support overrides while retaining defaults. Integration tests now use a local mock release server and document the fixture-based workflow. ChangesRelease service URL override
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: psalajova 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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/release/candidate/client.go (1)
45-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument both exported service-host APIs completely.
pkg/release/candidate/client.go#L45-L50: add GoDoc describing the endpoint produced and empty-versus-overriddenserviceHostbehavior.pkg/release/prerelease/client.go#L47-L55: document theclient,prerelease, andserviceHostparameters and the returned pullspec/error.As per coding guidelines, “Go documentation on Classes/Functions/Fields should be written properly.” As per path instructions, “Comment important exported functions with their purpose, parameters, and return values.”
🤖 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 `@pkg/release/candidate/client.go` around lines 45 - 50, Add complete GoDoc comments to the exported EndpointWithServiceHost function in pkg/release/candidate/client.go, describing the endpoint it builds and that an empty serviceHost uses ServiceHost(d) while a non-empty value overrides it. Also document the exported prerelease API in pkg/release/prerelease/client.go, covering the client, prerelease, and serviceHost parameters and the returned pullspec and error.Sources: Coding guidelines, Path instructions
🤖 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 `@test/integration/clusterimageset-updater.sh`:
- Around line 4-8: Update the Go mock startup flow around the command that
currently backgrounds go run: build the driver executable first, then background
that executable and assign its PID to mock_pid. Keep cleanup targeting mock_pid
so it reliably terminates the listener process.
---
Nitpick comments:
In `@pkg/release/candidate/client.go`:
- Around line 45-50: Add complete GoDoc comments to the exported
EndpointWithServiceHost function in pkg/release/candidate/client.go, describing
the endpoint it builds and that an empty serviceHost uses ServiceHost(d) while a
non-empty value overrides it. Also document the exported prerelease API in
pkg/release/prerelease/client.go, covering the client, prerelease, and
serviceHost parameters and the returned pullspec and error.
🪄 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)
Review profile: CHILL
Plan: Enterprise
Run ID: c03542aa-f74e-4f1b-ba30-604520bea7cd
📒 Files selected for processing (9)
cmd/clusterimageset-updater/main.gopkg/release/candidate/client.gopkg/release/prerelease/client.gotest/integration/clusterimageset-updater.shtest/integration/clusterimageset-updater/README.mdtest/integration/clusterimageset-updater/mock-release-server/main.gotest/integration/clusterimageset-updater/output/imagesets/ocp-release-4.21.25-multi-for-4.21.0-0-to-4.22.0-0_clusterimageset.yamltest/integration/clusterimageset-updater/output/pools/4-21-20_clusterpool.yamltest/integration/clusterimageset-updater/output/pools/4-21-auto_clusterpool.yaml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/release(manual)openshift/ci-docs(manual)openshift/release-controller(manual)openshift/ci-chat-bot(manual)
fcb9d35 to
feb5eac
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/release/prerelease/client.go (1)
51-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the exported resolver’s API contract.
ResolvePullSpecWithServiceURLis a new exported API, but its comment only states the purpose. Documentclient,prerelease, the empty/non-emptyserviceURLbehavior, and the returned pull spec/error.Suggested documentation
-// ResolvePullSpecWithServiceURL resolves a pull spec, optionally overriding the release service URL. +// ResolvePullSpecWithServiceURL resolves a pull spec for prerelease using client. +// A non-empty serviceURL overrides the default release service endpoint. +// It returns the resolved pull spec or an error.As per path instructions, exported functions must document their purpose, parameters, and return values.
🤖 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 `@pkg/release/prerelease/client.go` around lines 51 - 52, Expand the GoDoc for ResolvePullSpecWithServiceURL to document the client and prerelease inputs, explain that an empty serviceURL uses the default while a non-empty value overrides it, and describe the returned resolved pull spec and error.Source: Path instructions
🤖 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 `@test/integration/clusterimageset-updater/mock-release-server/main.go`:
- Line 43: Update main to stop printing the release-server endpoint URL to
stdout, preserving stdout as JSON-only for OTE; publish the endpoint through a
ready-file or stderr instead, and update the corresponding launcher to read the
endpoint from that new channel.
---
Nitpick comments:
In `@pkg/release/prerelease/client.go`:
- Around line 51-52: Expand the GoDoc for ResolvePullSpecWithServiceURL to
document the client and prerelease inputs, explain that an empty serviceURL uses
the default while a non-empty value overrides it, and describe the returned
resolved pull spec and error.
🪄 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)
Review profile: CHILL
Plan: Enterprise
Run ID: 1f2e227d-abee-4de6-9806-f67ed576e9a6
📒 Files selected for processing (7)
cmd/clusterimageset-updater/main.gopkg/release/candidate/client.gopkg/release/prerelease/client.gopkg/release/prerelease/client_test.gotest/integration/clusterimageset-updater.shtest/integration/clusterimageset-updater/README.mdtest/integration/clusterimageset-updater/mock-release-server/main.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/release(manual)openshift/ci-docs(manual)openshift/release-controller(manual)openshift/ci-chat-bot(manual)
🚧 Files skipped from review as they are similar to previous changes (3)
- test/integration/clusterimageset-updater.sh
- test/integration/clusterimageset-updater/README.md
- pkg/release/candidate/client.go
feb5eac to
8231671
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 `@test/integration/clusterimageset-updater/mock-release-server/main.go`:
- Line 56: Update the shutdown cleanup in main to handle the error returned by
server.Shutdown instead of discarding it. When shutdown fails, write the error
to stderr and terminate with a non-zero exit status; preserve the existing
successful shutdown behavior.
🪄 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)
Review profile: CHILL
Plan: Enterprise
Run ID: 61613d68-305c-41ba-b38d-cdb0622ef28f
📒 Files selected for processing (7)
cmd/clusterimageset-updater/main.gopkg/release/candidate/client.gopkg/release/prerelease/client.gopkg/release/prerelease/client_test.gotest/integration/clusterimageset-updater.shtest/integration/clusterimageset-updater/README.mdtest/integration/clusterimageset-updater/mock-release-server/main.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/release(manual)openshift/ci-docs(manual)openshift/release-controller(manual)openshift/ci-chat-bot(manual)
🚧 Files skipped from review as they are similar to previous changes (6)
- test/integration/clusterimageset-updater/README.md
- test/integration/clusterimageset-updater.sh
- pkg/release/prerelease/client_test.go
- pkg/release/candidate/client.go
- pkg/release/prerelease/client.go
- cmd/clusterimageset-updater/main.go
…ain.go Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Copy only input/output fixtures into the workdir and build the mock server to mock-release-server-bin so it does not clash with the source directory. Co-authored-by: Cursor <cursoragent@cursor.com>
os::cmd::expect_success requires an active junit suite. Build and start the mock release server after declare_suite_start instead. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@psalajova: 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. |
|
/retest-required |
1 participant
Summary
--release-service-hosttoclusterimageset-updaterfor overriding the OCP release catalog endpoint4.21.25-multi) instead of the live catalogtest/integration/clusterimageset-updater/README.mdDepends on #5322 (fixture bump to 4.21.25). Merge that first, then rebase this PR if needed.
Made with Cursor
Adds an optional
--release-service-urlCLI flag toclusterimageset-updaterto override the OCP release service base URL used when resolving prerelease pullspecs. This allows CI integration tests (and operators) to point the updater at a local or alternate release catalog instead of the live service.The release client code now supports resolving endpoints and pullspecs using the provided service URL while preserving the existing default behavior when the flag is unset. The
clusterimageset-updaterintegration suite starts a mock release server and passes--release-service-urlto ensure stable, catalog-independent results (mocking the pullspec to4.21.25-multi); fixture expectations are documented undertest/integration/clusterimageset-updater/README.md.This PR depends on
#5322being merged first.