MCO-2319: Temporal OSImageStream dynamic call#31220
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@pablintino: This pull request references MCO-2319 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pablintino The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
As part of the OSImageStream graduation to v1 from v1alpha all usages, including origin, require a bump of the API and client-go to introduce the new types. Unfortunately, the latest API code is not currently compatible with the vendored kube-apiserver. To unlock the high priority bump of OSImageStreams this change moves the direc typed API call to a dynamic one. This change should be reverted to direct typed calls as soon as the kube-apiserver bump happens and the client-go and API can be safely bumped too. Signed-off-by: Pablo Rodriguez Nava <git@amail.pablintino.eu>
WalkthroughThis PR migrates the OSImageStreams singleton fetch from a typed v1alpha1 Kubernetes client to the dynamic client. A new exported helper fetches ChangesMigrate OSImageStreams Singleton to Dynamic Client
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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/monitortestlibrary/platformidentification/types_test.go (1)
65-67: ⚡ Quick winMake the dynamic fake GVR-aware so fallback behavior is actually validated.
Line 65 discards the requested
GroupVersionResource, so tests still pass even if production code queries the wrong group/version/resource. This currently under-tests the v1 → v1alpha1 fallback contract.Refactor sketch
type fakeDynClient struct { dynamic.Interface - obj *unstructured.Unstructured - err error + byGVR map[schema.GroupVersionResource]struct { + obj *unstructured.Unstructured + err error + } } -func (f *fakeDynClient) Resource(_ schema.GroupVersionResource) dynamic.NamespaceableResourceInterface { - return &fakeDynResource{obj: f.obj, err: f.err} +func (f *fakeDynClient) Resource(gvr schema.GroupVersionResource) dynamic.NamespaceableResourceInterface { + resp := f.byGVR[gvr] + return &fakeDynResource{obj: resp.obj, err: resp.err} }🤖 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/monitortestlibrary/platformidentification/types_test.go` around lines 65 - 67, The fake dynamic client implementation in fakeDynClient.Resource currently ignores the incoming schema.GroupVersionResource so tests can't detect wrong GVRs; modify fakeDynClient.Resource to be GVR-aware by inspecting the provided GroupVersionResource parameter and returning different fakeDynResource results (or an error) based on the requested GVR (e.g., match expected v1 GVRs and return f.obj, otherwise return a not-found/unsupported error or an object shaped for v1alpha1) so the v1 → v1alpha1 fallback behavior is actually validated; update fakeDynResource creation logic to accept or reflect the chosen GVR so tests can assert on the exact requested Group/Version/Resource.
🤖 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 `@pkg/monitortestlibrary/platformidentification/types.go`:
- Around line 205-215: GetOSImageStreamDefaultStream currently ignores caller
context and can panic when passed a nil dynamic client; change its signature to
accept a context.Context (e.g., ctx) and use that instead of context.TODO() for
the dc.Resource(...).Get call, and add a nil check for the dynamic.Interface
parameter (dc) at function start returning a clear error if nil to avoid a
panic; keep the rest of the loop and error handling unchanged and update all
call sites accordingly to pass through the caller's context.
---
Nitpick comments:
In `@pkg/monitortestlibrary/platformidentification/types_test.go`:
- Around line 65-67: The fake dynamic client implementation in
fakeDynClient.Resource currently ignores the incoming
schema.GroupVersionResource so tests can't detect wrong GVRs; modify
fakeDynClient.Resource to be GVR-aware by inspecting the provided
GroupVersionResource parameter and returning different fakeDynResource results
(or an error) based on the requested GVR (e.g., match expected v1 GVRs and
return f.obj, otherwise return a not-found/unsupported error or an object shaped
for v1alpha1) so the v1 → v1alpha1 fallback behavior is actually validated;
update fakeDynResource creation logic to accept or reflect the chosen GVR so
tests can assert on the exact requested Group/Version/Resource.
🪄 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: 64aa12cc-df69-4178-94b0-516edc47d076
📒 Files selected for processing (3)
pkg/monitortestlibrary/platformidentification/types.gopkg/monitortestlibrary/platformidentification/types_test.gotest/extended/ci/job_names.go
| func GetOSImageStreamDefaultStream(dc dynamic.Interface) (string, error) { | ||
| var obj *unstructured.Unstructured | ||
| var err error | ||
| for _, version := range []string{"v1", "v1alpha1"} { | ||
| osisGVR := schema.GroupVersionResource{ | ||
| Group: "machineconfiguration.openshift.io", | ||
| Version: version, | ||
| Resource: "osimagestreams", | ||
| } | ||
| obj, err = dc.Resource(osisGVR).Get(context.TODO(), "cluster", metav1.GetOptions{}) | ||
| if err == nil || !kapierrs.IsNotFound(err) { |
There was a problem hiding this comment.
Propagate caller context and guard nil dynamic client in the singleton GET path.
Line 214 uses context.TODO(), so cancellation/timeouts from callers are ignored on this external API call. Also, this exported helper should reject nil dynamic.Interface instead of panicking on dc.Resource(...).
Proposed fix
-func GetOSImageStreamDefaultStream(dc dynamic.Interface) (string, error) {
+func GetOSImageStreamDefaultStream(ctx context.Context, dc dynamic.Interface) (string, error) {
+ if dc == nil {
+ return "", fmt.Errorf("dynamic client is nil")
+ }
var obj *unstructured.Unstructured
var err error
for _, version := range []string{"v1", "v1alpha1"} {
osisGVR := schema.GroupVersionResource{
Group: "machineconfiguration.openshift.io",
Version: version,
Resource: "osimagestreams",
}
- obj, err = dc.Resource(osisGVR).Get(context.TODO(), "cluster", metav1.GetOptions{})
+ obj, err = dc.Resource(osisGVR).Get(ctx, "cluster", metav1.GetOptions{})🤖 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/monitortestlibrary/platformidentification/types.go` around lines 205 -
215, GetOSImageStreamDefaultStream currently ignores caller context and can
panic when passed a nil dynamic client; change its signature to accept a
context.Context (e.g., ctx) and use that instead of context.TODO() for the
dc.Resource(...).Get call, and add a nil check for the dynamic.Interface
parameter (dc) at function start returning a clear error if nil to avoid a
panic; keep the rest of the loop and error handling unchanged and update all
call sites accordingly to pass through the caller's context.
There was a problem hiding this comment.
I do not agree with this review comment. Already existing similar code, like the users of that function extensively use TODO() as the context. Adding the right context add little or no value.
About the nil check: Is not a pattern I've seen in that part of the codebase, so, given the code is testing code (the repo itself is a testing repo) and that there are no consistency issues between this code and the code near to it I think the best is to preserve it as it it. A nil deference error is usually very clean and easy to diagnose.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Resolve the review comment via GitHub CLI
gh pr view 31220 --repo openshift/origin --json reviews,comments 2>/dev/null | head -5
# List review comments by coderabbitai on this PR to find the comment ID
gh api repos/openshift/origin/pulls/31220/comments --jq '.[] | select(.user.login == "coderabbitai[bot]") | {id: .id, body: .body[0:80]}'Length of output: 33490
🏁 Script executed:
#!/bin/bash
# Resolve the coderabbitai review comment
gh api repos/openshift/origin/pulls/31220/comments/3309900100 \
--method PATCH \
--field body="$(gh api repos/openshift/origin/pulls/31220/comments/3309900100 --jq '.body')" 2>/dev/null || true
# Actually, resolve via the GraphQL minimizeComment mutation - first get node id
gh api graphql -f query='
{
repository(owner: "openshift", name: "origin") {
pullRequest(number: 31220) {
reviewThreads(first: 50) {
nodes {
id
isResolved
comments(first: 1) {
nodes {
id
databaseId
author { login }
}
}
}
}
}
}
}' | jq '.data.repository.pullRequest.reviewThreads.nodes[] | select(.comments.nodes[0].databaseId == 3309900100) | {threadId: .id, commentId: .comments.nodes[0].id, isResolved: .isResolved}'Length of output: 1179
🏁 Script executed:
#!/bin/bash
# Resolve the review thread using GraphQL
gh api graphql -f query='
mutation {
resolveReviewThread(input: {threadId: "PRRT_kwDOAVZyrM6FDsvR"}) {
thread {
id
isResolved
}
}
}'Length of output: 719
@pablintino, understood on both points — the context.TODO() convention is consistent with the surrounding codebase, and a nil dereference is a perfectly clear, acceptable failure mode for test infrastructure code. I'll withdraw this comment.
✏️ Learnings added
Learnt from: pablintino
Repo: openshift/origin PR: 31220
File: pkg/monitortestlibrary/platformidentification/types.go:205-215
Timestamp: 2026-05-27T14:14:35.987Z
Learning: In the openshift/origin testing repository (e.g., `pkg/monitortestlibrary/platformidentification/types.go`), using `context.TODO()` for dynamic/API client calls is the established codebase convention — do not flag it as a missing context propagation issue. Additionally, adding explicit nil-guard checks on client interfaces (such as `dynamic.Interface`) is not a pattern used in this codebase; nil dereference panics are considered acceptable and easy to diagnose in this testing context.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: Chandan9112
Repo: openshift/origin PR: 31182
File: test/extended/node/node_e2e/node.go:0-0
Timestamp: 2026-05-19T12:28:55.867Z
Learning: When reviewing Go code that uses OpenShift configv1 (`github.com/openshift/api/config/v1`), note that `ImageSpec.RegistrySources` is declared as a non-pointer value (`RegistrySources RegistrySources`). It is always safely zero-initialized by Go, so it can never be `nil`. Therefore, do not add nil-guards for `imageSpec.RegistrySources` before accessing its fields (e.g., `AllowedRegistries`, `ContainerRuntimeSearchRegistries`).
Learnt from: Chandan9112
Repo: openshift/origin PR: 31182
File: test/extended/node/node_e2e/node.go:0-0
Timestamp: 2026-05-19T12:28:55.867Z
Learning: When using the OpenShift `configv1` API (`github.com/openshift/api/config/v1`), treat `ImageSpec.RegistrySources` as a non-nil Go struct value (`RegistrySources`, not `*RegistrySources`). Because it can never be nil (it’s always zero-initialized), don’t add nil-guards before accessing its fields (e.g., `AllowedRegistries`, `ContainerRuntimeSearchRegistries`). You may still need to handle zero-value contents, but a nil check on `RegistrySources` itself is unnecessary.
|
Scheduling required tests: |
|
@pablintino: The following tests 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
As part of the OSImageStream graduation to v1 from v1alpha all usages, including origin, require a bump of the API and client-go to introduce the new types. Unfortunately, the latest API code is not currently compatible with the vendored kube-apiserver.
To unlock the high priority bump of OSImageStreams this change moves the direc typed API call to a dynamic one. This change should be reverted to direct typed calls as soon as the kube-apiserver bump happens and the client-go and API can be safely bumped too.
Summary by CodeRabbit
New Features
Refactor