-
Notifications
You must be signed in to change notification settings - Fork 38
feat(integrations): refactor FanOut plugins #2441
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Jose I. Paris <jiparis@chainloop.dev>
Signed-off-by: Jose I. Paris <jiparis@chainloop.dev>
Signed-off-by: Jose I. Paris <jiparis@chainloop.dev>
Signed-off-by: Jose I. Paris <jiparis@chainloop.dev>
Signed-off-by: Jose I. Paris <jiparis@chainloop.dev>
@@ -0,0 +1,362 @@ | |||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you can find the generic Integration
logic
Signed-off-by: Jose I. Paris <jiparis@chainloop.dev>
Signed-off-by: Jose I. Paris <jiparis@chainloop.dev>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the plugins SDK by extracting FanOut-specific logic into dedicated interfaces while introducing a generic Integration
interface for future flexibility. The changes maintain API compatibility while improving code organization.
Key changes:
- Introduction of generic
Integration
interface andIntegrationBase
struct for common integration functionality - Refactoring of FanOut-specific logic into dedicated interfaces and structs that build upon the base integration primitives
- Making execution requests generic with payload-based architecture to support different integration types
Reviewed Changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 6 comments.
Show a summary per file
File | Description |
---|---|
app/controlplane/plugins/sdk/v1/integrations.go |
New file introducing base integration interfaces and utilities |
app/controlplane/plugins/sdk/v1/fanout.go |
Refactored to use new base integration and moved FanOut-specific logic |
app/controlplane/plugins/sdk/v1/plugin/grpc_server.go |
Updated to use new SDK structure and payload-based execution |
app/controlplane/plugins/sdk/v1/plugin/grpc_client.go |
Updated to handle new payload structure and material subscription methods |
Various core plugin implementations | Updated to work with new FanOutPayload structure |
Test files | Updated to use new SDK validation methods and mock structures |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Signed-off-by: Jose I. Paris <jiparis@chainloop.dev>
Signed-off-by: Jose I. Paris <jiparis@chainloop.dev>
Signed-off-by: Jose I. Paris <jiparis@chainloop.dev>
Signed-off-by: Jose I. Paris <jiparis@chainloop.dev>
Signed-off-by: Jose I. Paris <jiparis@chainloop.dev>
Signed-off-by: Jose I. Paris <jiparis@chainloop.dev>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
envelope, err := testEnvelope("testdata/attestation.json") | ||
require.NoError(s.T(), err) | ||
|
||
s.ociIntegrationBackend.(*mockedSDK.FanOut).On("IsSubscribedTo", "CONTAINER_IMAGE").Return(true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need this mocks now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because these used to be properties, but now they are methods in the FanOut interface. The new implementation calls them to get the same values.
// Mocked integration that will return both generic configuration and credentials | ||
integration := integrationMocks.NewFanOut(s.T()) | ||
type schema struct { | ||
FirstName string `json:"firstName"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's firstName for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s.configStruct
used the tests has this property. It would fail with validation error: additionalProperties 'firstName' not allowed
if it's not specified in the schema.
|
||
// Methods to be implemented by the specific integration | ||
|
||
func (i *FanOutIntegration) Register(_ context.Context, _ *RegistrationRequest) (*RegistrationResponse, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice.
This PR refactors the plugins SDK to extract the specifics for FanOut plugins to its own logic, keeping a base
Integration
interface for future improvements.Integration
interface andIntegrationBase
struct for generic integrations. Integrations can be of multiple kinds (fanout, notification)FanOutIntegration
struct to use the new plugins primitives, and moved all FanOut-specific logic to these interfaces.NewFanOut
) have not changed.The external interfaces have not changed, APIs and database tables remain untouched.
Before:

After:
