Add policy client and protocol plumbing#38514
Conversation
dsa0x
added
the
no-changelog-needed
SarahFrench
left a comment
SarahFrench
left a comment
There was a problem hiding this comment.
👋🏻 I've been taking a look at this today and will resume tomorrow, but here are some initial comments that mainly revolve around testing.
| } | ||
|
|
||
| func RegisterCallbackServiceServer(s grpc.ServiceRegistrar, srv CallbackServiceServer) { | ||
| // If the following call pancis, it indicates UnimplementedCallbackServiceServer was |
There was a problem hiding this comment.
| // If the following call pancis, it indicates UnimplementedCallbackServiceServer was | |
| // If the following call panics, it indicates UnimplementedCallbackServiceServer was |
| } | ||
| } | ||
|
|
||
| func (c *client) Evaluate(ctx context.Context, req EvaluationRequest[*proto.ResourceMetadata]) EvaluationResponse { |
There was a problem hiding this comment.
Given the existence of EvaluateModule and EvaluateProvider, what do you think about renaming this EvaluateResource? I think this would make the code resemble the proto definition, too.
| return s.evaluateModuleFn(req) | ||
| } | ||
|
|
||
| func TestClientEvaluate(t *testing.T) { |
There was a problem hiding this comment.
I think there can be more test coverages here, both for this and the other 2 Evaluate-type methods.
For example:
- You could have the
evaluateResourceFnreturn an empty or nil response alongside a non-nil error - does the response fromEvaluatehandle that error as expected? - You could also include more data in the response from
evaluateResourceFnand make more assertions about how it's transformed by the Evaluate method. E.g. if you include somePolicyDetailsdata and assert that diagnostics from in there are transformed as expected into diagnostics in the response fromEvaluate.
| return diags | ||
| } | ||
|
|
||
| func (diagnostic *Diagnostic) ToHCL() *hcl.Diagnostic { |
There was a problem hiding this comment.
Could this benefit from some test coverage?
I know the concept of 'diagnostic extras' isn't new to the codebase, but this implementation is and it might be worth adding some light tests asserting that these behave in the way we expect.
| }, nil | ||
| }, | ||
| }, | ||
| callbackRegistry: callback.NewRegistry(), |
There was a problem hiding this comment.
Idea: could you make a mock callback registry to use in tests like this?
2 participants
This is part of a stacked series to upstream the policy work in smaller, reviewable pieces:
This PR introduces the policy package and the basic protocol/client plumbing that the rest of the stack builds on.
The goal here is to land the policy-specific infrastructure first so the later PRs can focus on runtime behavior.
Included here
Target Release
1.16.x
Rollback Plan
Changes to Security Controls
Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.
CHANGELOG entry