-
Notifications
You must be signed in to change notification settings - Fork 25
adding grpc workflow metadata source client and related types #1749
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
✅ API Diff Results - No breaking changes |
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.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.
| Benchmark suite | Current: 6872c33 | Previous: 6c0b1c8 | Ratio |
|---|---|---|---|
BenchmarkKeystore_Sign/nop/in-process |
770.9 ns/op |
358.4 ns/op |
2.15 |
This comment was automatically generated by workflow using github-action-benchmark.
pkg/workflows/grpcsource/client.go
Outdated
| // start is the pagination offset (0-indexed). | ||
| // limit is the maximum number of workflows to return per page. | ||
| // Returns workflows, head, hasMore flag indicating if more pages exist, and error. | ||
| func (c *Client) ListWorkflowMetadata(ctx context.Context, families []string, start, limit int64) ([]*pb.WorkflowMetadata, *pb.Head, bool, 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.
@patrickhuie19 Are you sure you want to put this logic in a separate repository? That seems like a heavy price to pay -- isn't the only consumer of this client going to be in core?
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.
Thanks cedric, good q: the benefits I'm seeing: it allows us to easily inject the client into smoke tests within the chainlink repo and eventually the centralized component to deploy in. We follow this pattern for the storage and billing clients.
d2e620e to
e53998d
Compare
| AddWorkflow(ctx context.Context, workflow *WorkflowRegistration) error | ||
|
|
||
| // UpdateWorkflow updates the workflow's status configuration | ||
| // This is idempotent - calling with the same config multiple times has the same effect |
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.
nit: I think this goes without saying, especially when looking at WorkflowStatusConfig struct
pkg/workflows/grpcsource/client.go
Outdated
| return nil, fmt.Errorf("failed to create JWT: %w", err) | ||
| } | ||
|
|
||
| return metadata.AppendToOutgoingContext(ctx, "authorization", "Bearer "+jwtToken), nil |
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.
TIL: My gut reaction was that we don't need the "Bearer" prefix in gRPC as it is only specific to HTTP. But a quick Google search said otherwise :)
pkg/workflows/grpcsource/client.go
Outdated
| return nil, fmt.Errorf("failed to create JWT: %w", err) | ||
| } | ||
|
|
||
| return metadata.AppendToOutgoingContext(ctx, "authorization", "Bearer "+jwtToken), nil |
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.
nit: put those in consts?
| // AuthorizationHeader is the lowercase header key for authorization | ||
| AuthorizationHeader = "authorization" | ||
| // BearerPrefix is the prefix for Bearer tokens | ||
| BearerPrefix = "Bearer " |
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.
nit: are you sure you want the space included in the constant?
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.
That is defined from the protocol definition, so yes I think it's desirable - what downside are you seeing?
Supporting Private Workflow Registry
This change is part of a series of changes to allow the workflow engine to pull workflows from metadata sources other than the WorkflowRegistry.
For broader context, check out https://docs.google.com/document/d/1K5lLtczMe_HyrLTbdLNmb_jxj40lPCmMZW4OBjn7K5U/edit?tab=t.t17fuwqoj0w9#bookmark=id.i3vs01f9lvvr
For the workflow engine specific changes, this diagram is most relevant:

Series
[In-review] chainlink-protos: smartcontractkit/chainlink-protos#255
[In-review] chainlink-common: #1749
[In-review] chainlink: smartcontractkit/chainlink#20708
Changes specific to chainlink-common