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.
| 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.
There was a problem hiding this comment.
➕ to that idea, I was also looking at the proto definition of diagnostic and it's a little difficult to understand exactly what data we will receive in a diagnostic. Perhaps some tests could serve as documentation of "these are some examples of what this extra info will look like"
There was a problem hiding this comment.
👍
I did have more extensive tests in subsequent PRs that exercise assertions around the different kinds of policy results and diagnostics. Putting them further up the stack just helped that one test exercises a lot more paths. But I understand that context is kinda lost in this task break down. I have added some more here for this unit.
austinvalle
left a comment
austinvalle
left a comment
There was a problem hiding this comment.
Very nice 👍🏻, I appreciate you breaking these out 😆. I left some comments and questions!
- More testing for the callbacks and policy client - setup a mock callback registry - Rename Evaluate to EvaluateResource
- More comments - Use optional for Snippet
dsa0x
force-pushed
the
policy-pr1-foundation
branch
from
cc00cb2 to
9c5c4f8
Compare
dsa0x
force-pushed
the
policy-pr1-foundation
branch
from
9c5c4f8 to
a607df3
Compare
3 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