Allow users to get/delete templates by name only #89
Conversation
Signed-off-by: Max Smythe <smythe@google.com>
Signed-off-by: Max Smythe <smythe@google.com>
maxsmythe
force-pushed
the
get-template-by-name-only
branch
from
cd8939a
to
183ad84
Compare
Signed-off-by: Max Smythe <smythe@google.com>
Signed-off-by: Max Smythe <smythe@google.com>
Signed-off-by: Max Smythe <smythe@google.com>
Signed-off-by: Max Smythe <smythe@google.com>
| @@ -24,13 +24,13 @@ import ( | |||
|
|
|||
| const constraintGroup = "constraints.gatekeeper.sh" | |||
|
|
|||
| type ClientOpt func(*Client) error | |||
| type Opt func(*Client) error | |||
There was a problem hiding this comment.
Do we have any compatibility guarantees that would discourage such an API change? Is Gatekeeper the primary consumer of the Constraint Framework?
There was a problem hiding this comment.
Correct. I haven't added semver to leave room for stuff like this. Further this should be mostly transparent as ClientOpt/Opt are only used as inputs to the creation functions.
Forseti is the only other consumer of the CF that I am aware of: https://github.com/forseti-security/config-validator
| expectedCount = 1 | ||
| expectedHandled = map[string]bool{"h1": true} | ||
| } | ||
| if r.HandledCount() != expectedCount { |
There was a problem hiding this comment.
It's not clear whether r is valid and non-nil on this line if we received an error from RemoveTemplate above.
There was a problem hiding this comment.
r is always returned as other handlers may have had the chance to process before an error was raised.
There was a problem hiding this comment.
If that's the case I might suggest some combination of these:
- Returning the responses as a value rather than a pointer
- Checking for nil receiver in
HandledCountandResults - Documenting that responses is valid even upon error
I generally assume return values are invalid when an error is returned, which is also echoed here: https://github.com/golang/go/wiki/CodeReviewComments#in-band-errors
Instead of requiring clients to check for an in-band error value, a function should return an additional value to indicate whether its other return values are valid. This return value may be an error, or a boolean when no explanation is needed. It should be the final return value.
...but I recognize the need to report additional information on partial success.
There was a problem hiding this comment.
Re: value instead of pointer...
Given that the amount of memory it takes to represent this object potentially scales with the number of responses, I'm wary of using a value. Since it's currently represented by a map, that may not be an issue, though I like the idea of guaranteeing we are only ever copying-by-reference.
Re: checking for a nil receiver...
That's a good idea, then the helper functions could return empty results
Re: documentation... ya, there is definitely interface documentation work to be done
Looking through that doc, the "how to do errors" gets a bit hand-wavy at points b/c they can't predict all uses. I think this is one. I think what we have strikes a reasonable balance between the two competing needs and follows the main point, which is "report errors out-of-band to avoid the need for introspection"
Note that if a user is not interested in the result if an error is returned, usage of this function devolves into standard Go code.
There was a problem hiding this comment.
Added the checking for a nil receiver and documented that results are still valid even when error is returned.
Signed-off-by: Max Smythe <smythe@google.com>
|
Once this is merged, can we pls vendor the latest to gk? :) |
Signed-off-by: Max Smythe smythe@google.com