Draft: Add srelib integration POC#839
Draft
RaphaelBut wants to merge 1 commit into
Draft
Conversation
Demonstrate how to include and use the srelib plugin library in CAD. Adds an adapter that wraps srelib's v1.Client behind CAD's ocm.Client interface, a plugin launcher for managing the srelib process lifecycle, and a runnable example showing the end-to-end flow. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
WalkthroughThis PR introduces plugin-based integration of srelib as an ocm.Client implementation. It upgrades dependencies for plugin management, implements a subprocess launcher for srelib, wraps the plugin client to expose the ocm.Client interface, provides adapter tests, and demonstrates end-to-end usage through a POC. ChangesSRElib Plugin Integration for OCM Client
🎯 3 (Moderate) | ⏱️ ~20 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (3 errors, 2 warnings, 1 inconclusive)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: RaphaelBut 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
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
@RaphaelBut: 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/srelib/launcher.go (1)
58-60: ⚡ Quick winConsider adding graceful shutdown before Kill().
The
Close()method immediately kills the plugin process. Consider attempting a graceful shutdown first (e.g., callingExited()or setting a timeout) before falling back toKill()to allow the plugin to clean up resources.💡 Example graceful shutdown pattern
func (p *PluginClient) Close() { + // Request graceful exit, then kill if it doesn't exit + select { + case <-p.raw.Exited(): + return + case <-time.After(5 * time.Second): + p.raw.Kill() + } - p.raw.Kill() }🤖 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/srelib/launcher.go` around lines 58 - 60, The Close() method currently calls p.raw.Kill() immediately; change it to attempt a graceful shutdown first by signaling or waiting for the process to exit (e.g., call p.raw.Signal / p.raw.Wait / Exited() if available) with a bounded timeout, and only call p.raw.Kill() as a fallback if the process does not exit within that timeout; update PluginClient.Close to perform the graceful stop + timeout logic around the existing p.raw.Kill() call so the plugin has a chance to cleanup before being killed.pkg/srelib/adapter.go (1)
40-42: 💤 Low valueSilent failure for missing subscription may hide errors.
When the cluster has no subscription, the method returns an empty string with
nilerror. Depending on CAD's expectations, a missing subscription might indicate a data integrity issue that should be surfaced as an error rather than silently returning empty.Alternative approach
cmv1Sub, ok := cluster.GetSubscription() if !ok { - return "", nil + return "", fmt.Errorf("cluster %s has no subscription", clusterID) }🤖 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/srelib/adapter.go` around lines 40 - 42, The code silently returns an empty string and nil error when cluster.GetSubscription() reports no subscription (cmv1Sub, ok := cluster.GetSubscription()); change this to return a non-nil error instead of nil so callers can detect the missing subscription (e.g., return "", fmt.Errorf("missing subscription for cluster") or a typed error), and update any callers or tests that expect the previous silent behavior; locate the check for ok after cluster.GetSubscription() in pkg/srelib/adapter.go to implement this change.
🤖 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 `@go.mod`:
- Line 29: The go.mod contains a replace directive pointing to a local path
("../srelib") that does not exist and will break builds; either remove the
replace directive from go.mod or replace it with a valid module version (e.g.,
use the published module version for github.com/petrkotas/srelib) and, if you
need to keep local development behavior, add a README note instructing
contributors to clone srelib as a sibling directory or run 'go mod edit
-replace' locally—update the replace entry in go.mod or the project
documentation accordingly.
In `@pkg/srelib/adapter.go`:
- Around line 79-81: The GetConnection method on Adapter currently returns nil
which causes downstream panics; implement and return a stub connection that
satisfies the sdk.Connection interface and returns explicit errors for
operations (e.g., TokensContext) so callers get clear failures instead of nil
derefs. Create a type (e.g., sreStubConnection) in the same package that
implements the sdk.Connection methods used by consumers and returns
context-aware errors, then have (*Adapter).GetConnection return
&sreStubConnection{}; ensure the error messages clearly state that srelib does
not support OCM SDK connections to aid debugging.
---
Nitpick comments:
In `@pkg/srelib/adapter.go`:
- Around line 40-42: The code silently returns an empty string and nil error
when cluster.GetSubscription() reports no subscription (cmv1Sub, ok :=
cluster.GetSubscription()); change this to return a non-nil error instead of nil
so callers can detect the missing subscription (e.g., return "",
fmt.Errorf("missing subscription for cluster") or a typed error), and update any
callers or tests that expect the previous silent behavior; locate the check for
ok after cluster.GetSubscription() in pkg/srelib/adapter.go to implement this
change.
In `@pkg/srelib/launcher.go`:
- Around line 58-60: The Close() method currently calls p.raw.Kill()
immediately; change it to attempt a graceful shutdown first by signaling or
waiting for the process to exit (e.g., call p.raw.Signal / p.raw.Wait / Exited()
if available) with a bounded timeout, and only call p.raw.Kill() as a fallback
if the process does not exit within that timeout; update PluginClient.Close to
perform the graceful stop + timeout logic around the existing p.raw.Kill() call
so the plugin has a chance to cleanup before being killed.
🪄 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: 45add7dd-2e8a-41ee-847d-10acdf9bdac2
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
go.modpkg/srelib/adapter.gopkg/srelib/adapter_test.gopkg/srelib/launcher.gopoc/srelib-integration/main.go
| github.com/openshift/hive/apis v0.0.0-20260430175100-52fd62d6be80 | ||
| github.com/openshift/osd-network-verifier v1.7.0 | ||
| github.com/openshift/osde2e-common v0.0.0-20260421185005-d390433565e5 | ||
| github.com/petrkotas/srelib v0.0.0 |
There was a problem hiding this comment.
Replace directive points to non-existent directory.
The replace directive references ../srelib, which does not exist in the repository. This will break builds for anyone cloning the repo. For a POC that requires local development with an external module, document the required setup steps in a README or remove the replace directive before sharing.
📝 Suggested approach
For POC/development purposes, either:
- Document the setup requirement (e.g., "Clone srelib as a sibling directory")
- Use a proper version reference if srelib is published
- Add a note in the PR description about the local setup requirement
Also applies to: 230-230
🤖 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 `@go.mod` at line 29, The go.mod contains a replace directive pointing to a
local path ("../srelib") that does not exist and will break builds; either
remove the replace directive from go.mod or replace it with a valid module
version (e.g., use the published module version for github.com/petrkotas/srelib)
and, if you need to keep local development behavior, add a README note
instructing contributors to clone srelib as a sibling directory or run 'go mod
edit -replace' locally—update the replace entry in go.mod or the project
documentation accordingly.
Source: Linters/SAST tools
Comment on lines
+79
to
+81
| func (a *Adapter) GetConnection() *sdk.Connection { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
GetConnection() returning nil will cause downstream panics.
The GetConnection() method returns nil, but downstream CAD code dereferences the connection without nil checks. For example, pkg/backplane/backplane.go:74 calls config.OcmClient.GetConnection().TokensContext(ctx), which will panic with a nil pointer dereference.
🔧 Possible approaches
- Document the limitation: If srelib-backed clients should not use backplane, add a runtime check or document which investigations are compatible.
- Return an error-wrapped stub: Return a connection that returns errors for token operations.
- Panic early with clear message: Panic in GetConnection() with a message explaining srelib doesn't support OCM SDK connections.
🤖 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/srelib/adapter.go` around lines 79 - 81, The GetConnection method on
Adapter currently returns nil which causes downstream panics; implement and
return a stub connection that satisfies the sdk.Connection interface and returns
explicit errors for operations (e.g., TokensContext) so callers get clear
failures instead of nil derefs. Create a type (e.g., sreStubConnection) in the
same package that implements the sdk.Connection methods used by consumers and
returns context-aware errors, then have (*Adapter).GetConnection return
&sreStubConnection{}; ensure the error messages clearly state that srelib does
not support OCM SDK connections to aid debugging.
Contributor
|
PR needs rebase. 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. |
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
1 participant
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.
Demonstrate how to include and use the srelib plugin library in CAD. Adds an adapter that wraps srelib's v1.Client behind CAD's ocm.Client interface, a plugin launcher for managing the srelib process lifecycle, and a runnable example showing the end-to-end flow.
What type of PR is this?
draft POC, not to be merged
( fully vibed by claude, no thinking^^ )
Summary by CodeRabbit
Release Notes
Chores
New Features
Tests