Repository navigation
chore: update ocm-cli to v1.0.17 - #1010
openshift-merge-bot[bot] merged 1 commit into
Conversation
Update ocm-cli from 1.0.10 to 1.0.17, align the Kubernetes and OpenShift client modules, and preserve pre-4.12 ConsolePlugin support through the dynamic client.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 9 remain after this review. WalkthroughThe pre-4.12 console-plugin lookup now reads ConsolePlugin resources through the Kubernetes dynamic client and builds enabled plugin URLs from unstructured service fields. Missing or invalid fields return field-specific errors. Tests cover URL construction, plugin filtering, and API errors. Dependency versions are updated. ChangesLegacy Console Plugin Lookup
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Other Sequence Diagram(s)sequenceDiagram
participant Lookup as getConsolePluginFrom411Cluster
participant KubernetesAPI as Kubernetes API
participant OperatorAPI as Console operator API
participant URLHelper as consolePluginV1Alpha1URL
Lookup->>KubernetesAPI: List ConsolePlugin resources
KubernetesAPI-->>Lookup: Return plugin list
Lookup->>OperatorAPI: Check plugin enablement
OperatorAPI-->>Lookup: Return enablement result
Lookup->>URLHelper: Build URL for enabled plugin
URLHelper-->>Lookup: Return service URL or field error
Merge Risk: ⚪ Minimal · up to The legacy plugin lookup has no identified issue blocking merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@redhat-chai-bot: 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. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1010 +/- ##
==========================================
+ Coverage 55.86% 56.41% +0.55%
==========================================
Files 78 78
Lines 6159 6184 +25
==========================================
+ Hits 3441 3489 +48
+ Misses 2311 2286 -25
- Partials 407 409 +2
🚀 New features to boost your workflow:
|
| func consolePluginV1Alpha1URL(consolePlugin unstructured.Unstructured) (string, error) { | ||
| serviceName, found, err := unstructured.NestedString(consolePlugin.Object, "spec", "service", "name") | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to read spec.service.name from console plugin %q: %w", consolePlugin.GetName(), err) | ||
| } | ||
| if !found { | ||
| return "", fmt.Errorf("console plugin %q is missing spec.service.name", consolePlugin.GetName()) | ||
| } | ||
|
|
||
| serviceNamespace, found, err := unstructured.NestedString(consolePlugin.Object, "spec", "service", "namespace") | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to read spec.service.namespace from console plugin %q: %w", consolePlugin.GetName(), err) | ||
| } | ||
| if !found { | ||
| return "", fmt.Errorf("console plugin %q is missing spec.service.namespace", consolePlugin.GetName()) | ||
| } | ||
|
|
||
| servicePort, found, err := unstructured.NestedInt64(consolePlugin.Object, "spec", "service", "port") | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to read spec.service.port from console plugin %q: %w", consolePlugin.GetName(), err) | ||
| } | ||
| if !found { | ||
| return "", fmt.Errorf("console plugin %q is missing spec.service.port", consolePlugin.GetName()) | ||
| } | ||
|
|
||
| basePath, found, err := unstructured.NestedString(consolePlugin.Object, "spec", "service", "basePath") | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to read spec.service.basePath from console plugin %q: %w", consolePlugin.GetName(), err) | ||
| } | ||
| if !found { | ||
| return "", fmt.Errorf("console plugin %q is missing spec.service.basePath", consolePlugin.GetName()) | ||
| } | ||
|
|
||
| return fmt.Sprintf("%s=https://%s.%s.svc.cluster.local:%d%s", | ||
| consolePlugin.GetName(), serviceName, serviceNamespace, servicePort, basePath), nil | ||
| } | ||
|
|
There was a problem hiding this comment.
@redhat-chai-bot this is a large implementation for a resource which is simple originally. Is there a way to simplify it? Is implementing it in dynamic a must? instead of using the original client-go one.
There was a problem hiding this comment.
Thanks for raising this. I checked the exact client revisions in this PR:
- The older
github.com/openshift/client-gorevision contains the typedconsole/.../v1alpha1client, but the compatible revision selected for theocm-cli v1.0.17dependency graph no longer contains that package; it retains the typed ConsolePlugin client underconsole/.../v1. ocm-cli v1.0.17requiresk8s.io/apimachinery v0.34.3. The older OpenShift client-go module is tied to Kubernetes 0.25 andstructured-merge-diff/v4, while the compatible graph uses Kubernetes 0.34 andstructured-merge-diff/v6. Restoring the older typed client would reintroduce the incompatibility that caused the original dependency-update build to fail.- The legacy cluster resource is still
console.openshift.io/v1alpha1, so this path uses the dynamic client with an explicit GVR and a small helper to read the four service fields. The tests cover the GVR, JSON decoding, enabled/disabled filtering, API errors, and invalid fields.
For this dependency set, the dynamic client is necessary; reverting to the old typed client is not viable without reverting the compatible client dependency graph. The helper can still be simplified structurally if there is a preferred local pattern, but the client choice is driven by the API/module change.
AI-generated. Review for accuracy.
|
/hold I need to run a local test to validate e2e. |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/hold cancel I built this PR locally and the monitoring console plugin can still work. Although this PR adds a big implementation for the plugin itself, the logic is clear, I accept it. |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: feichashao, redhat-chai-bot 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
Summary
github.com/openshift-online/ocm-clifrom v1.0.10 to v1.0.17.Validation
make testgo vet ./...make lintmake buildgo mod verifygo mod tidywith no dependency-file diffgofmtandgit diff --checkAI-generated. Review for accuracy.
@feichashao requested from Slack
Summary by CodeRabbit