clusterimageset-updater: replace integration test with unit test - #5340
Open
Prucek wants to merge 3 commits into
Open
clusterimageset-updater: replace integration test with unit test#5340Prucek wants to merge 3 commits into
Prucek wants to merge 3 commits into
Conversation
Contributor
|
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 now accepts an optional release-controller URL, uses configurable endpoint helpers for pullspec resolution, and tests the override with a local mock server. Expected ClusterImageSet and ClusterPool manifests now reference release 4.21.99. ChangesRelease controller override
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 13 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Prucek 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
Prucek
force-pushed
the
fix-clusterimageset-updater-test
branch
2 times, most recently
from
July 29, 2026 12:20
45233e4 to
59f7642
Compare
Prucek
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: 1
🧹 Nitpick comments (1)
pkg/release/candidate/client.go (1)
46-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument both exported endpoint helpers consistently.
Both new
EndpointWithBasefunctions describe their purpose but omit parameter and return-value documentation.
pkg/release/candidate/client.go#L46-L53: documentd,version,stream,suffix,baseURL, and the returned endpoint string.pkg/release/prerelease/client.go#L22-L31: documentp,baseURL, and the returned endpoint string.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 46 - 53, Expand the documentation for the exported EndpointWithBase function in pkg/release/candidate/client.go (lines 46-53) to describe d, version, stream, suffix, baseURL, and the returned endpoint string. Apply the same documentation improvement to EndpointWithBase in pkg/release/prerelease/client.go (lines 22-31), covering p, baseURL, and the returned endpoint string; no implementation changes are needed.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.sh`:
- Around line 42-48: Update the mock-server readiness loop to use a checked curl
invocation that only succeeds for successful HTTP responses, and retain the loop
counter after retries. After the loop, explicitly fail the test unless the
counter confirms the server became ready; reference the existing readiness loop
and its i variable.
---
Nitpick comments:
In `@pkg/release/candidate/client.go`:
- Around line 46-53: Expand the documentation for the exported EndpointWithBase
function in pkg/release/candidate/client.go (lines 46-53) to describe d,
version, stream, suffix, baseURL, and the returned endpoint string. Apply the
same documentation improvement to EndpointWithBase in
pkg/release/prerelease/client.go (lines 22-31), covering p, baseURL, and the
returned endpoint string; no implementation changes are needed.
🪄 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: f1484d06-0138-4f69-9a86-b450b35f85d9
📒 Files selected for processing (7)
cmd/clusterimageset-updater/main.gopkg/release/candidate/client.gopkg/release/prerelease/client.gotest/integration/clusterimageset-updater.shtest/integration/clusterimageset-updater/output/imagesets/ocp-release-4.21.99-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)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/integration/clusterimageset-updater/output/pools/4-21-auto_clusterpool.yaml
- test/integration/clusterimageset-updater/output/pools/4-21-20_clusterpool.yaml
Prucek
force-pushed
the
fix-clusterimageset-updater-test
branch
from
July 29, 2026 12:34
59f7642 to
ff42fed
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.sh`:
- Line 21: Update the mock-server setup around MOCK_PORT so the Python server
binds to port 0 itself, retains the bound socket while starting the server, and
communicates the assigned port back to the shell; remove the separate
socket-selection and early-close flow to prevent the port from being claimed
between selection and binding.
🪄 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: fbea1e7b-b9a3-49ad-9e35-878a238a0047
📒 Files selected for processing (7)
cmd/clusterimageset-updater/main.gopkg/release/candidate/client.gopkg/release/prerelease/client.gotest/integration/clusterimageset-updater.shtest/integration/clusterimageset-updater/output/imagesets/ocp-release-4.21.99-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)
🚧 Files skipped from review as they are similar to previous changes (6)
- test/integration/clusterimageset-updater/output/pools/4-21-20_clusterpool.yaml
- pkg/release/prerelease/client.go
- test/integration/clusterimageset-updater/output/pools/4-21-auto_clusterpool.yaml
- test/integration/clusterimageset-updater/output/imagesets/ocp-release-4.21.99-multi-for-4.21.0-0-to-4.22.0-0_clusterimageset.yaml
- cmd/clusterimageset-updater/main.go
- pkg/release/candidate/client.go
droslean
reviewed
Jul 29, 2026
| expected="${workdir}/output" | ||
|
|
||
| # Start a mock release controller that returns a deterministic version. | ||
| MOCK_PORT=$(python3 -c 'import socket; s=socket.socket(); s.bind(("",0)); print(s.getsockname()[1]); s.close()') |
Member
There was a problem hiding this comment.
I don't think that you should do that.... I guess your AI is trying to hack the hell out of it.
Prucek
force-pushed
the
fix-clusterimageset-updater-test
branch
from
July 29, 2026 13:30
ff42fed to
98365a2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/release/prerelease/client.go (1)
22-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComplete the GoDoc for the exported helper.
The comment explains the purpose and
baseURL, but not whatpsupplies or what string is returned. Make the public contract explicit.Suggested documentation
// EndpointWithBase constructs the release controller endpoint for a prerelease // using the given base URL instead of the default service host. +// p provides the prerelease release descriptor and version bounds. +// baseURL overrides the default release-controller host. +// It returns the release-controller endpoint URL. func EndpointWithBase(p api.Prerelease, baseURL string) string {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/prerelease/client.go` around lines 22 - 24, Complete the GoDoc for the exported EndpointWithBase function by documenting what the p parameter represents and describing the endpoint string it returns, while retaining the existing purpose and baseURL explanation.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-controller/main.go`:
- Line 34: Handle the error returned by http.Serve in the mock controller’s main
serving flow instead of ignoring it. Report the failure with informative context
to stderr and exit with a nonzero status when serving stops unexpectedly, while
preserving the existing successful startup behavior.
---
Nitpick comments:
In `@pkg/release/prerelease/client.go`:
- Around line 22-24: Complete the GoDoc for the exported EndpointWithBase
function by documenting what the p parameter represents and describing the
endpoint string it returns, while retaining the existing purpose and baseURL
explanation.
🪄 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: 8a278ac3-9b2f-40f3-8777-4729d85e6059
📒 Files selected for processing (8)
cmd/clusterimageset-updater/main.gopkg/release/candidate/client.gopkg/release/prerelease/client.gotest/integration/clusterimageset-updater.shtest/integration/clusterimageset-updater/mock-release-controller/main.gotest/integration/clusterimageset-updater/output/imagesets/ocp-release-4.21.99-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)
🚧 Files skipped from review as they are similar to previous changes (5)
- test/integration/clusterimageset-updater/output/pools/4-21-auto_clusterpool.yaml
- test/integration/clusterimageset-updater/output/imagesets/ocp-release-4.21.99-multi-for-4.21.0-0-to-4.22.0-0_clusterimageset.yaml
- pkg/release/candidate/client.go
- test/integration/clusterimageset-updater/output/pools/4-21-20_clusterpool.yaml
- cmd/clusterimageset-updater/main.go
Prucek
force-pushed
the
fix-clusterimageset-updater-test
branch
from
July 29, 2026 13:42
98365a2 to
41a89e1
Compare
Extract the core logic into a run(options, HTTPClient) function so it can be called from unit tests with a mock HTTP client. main() now only parses flags, validates, and calls run(). Add EndpointWithBase to candidate and prerelease packages and a --release-controller-url flag to support URL override for testing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use httptest.NewServer to serve a deterministic fake release (4.21.99) so the test never breaks when a new z-stream is published. Input and expected output are in testdata/run/. Covers imageset creation, outdated imageset deletion, pool ref updates, and label propagation. Also move the inline expected YAML in TestEnsureLabelsOnClusterPools to a testdata file. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The integration test hit the real release controller API which made it break on every z-stream release. The new unit test for run() with httptest.NewServer covers the same functionality without external dependencies. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Prucek
force-pushed
the
fix-clusterimageset-updater-test
branch
from
July 29, 2026 14:40
41a89e1 to
80c8b2a
Compare
Prucek
changed the title
Contributor
|
Scheduling tests matching the |
Contributor
|
@Prucek: all tests passed! 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
clusterimageset-updaterintegration test hit the real release controller API, so it broke every time a new z-stream was published.run(options, HTTPClient)function.--release-controller-urlflag andEndpointWithBasehelpers to support URL override.httptest.NewServerwithtesthelper.CompareWithFixturegolden files — deterministic, no external dependencies.