feat(update): add update attributes handler and Cobra CLI cmd #4
Conversation
…proved flag rule value validation to prevent grpc response error, and also add notes in TODOs and comments about a missing attribute ID source for the Update call and questions about implementing attribute group CRUD and how we can possibly clean up Update so there are not a dozen flags with some containing lists as values
…al cursor to new line after rendering. Make string slice flag function more reusable by changing naming from 'required' to just get the slice with a possible 0 minimum in options argument passed. Render Id in List and Get tables rendered on success.
| fmt.Println(ErrorMessage("Flag "+flag+" is required", nil)) | ||
| os.Exit(1) | ||
| } | ||
| // if v == 0 { |
There was a problem hiding this comment.
Will we ever allow zero's as our int32 values passed by users? For example, I ran into an issue here with resource-version. If that value must be defined by a User in an Attribute - Update flow, the version should currently be truly 0, but 0 is also the Go zero-value for the int32 type, so if we want to require an int32 flag, we'd want to gracefully handle these zero values.
There was a problem hiding this comment.
I think if the user has to apply the version that is a problem. It seems like the version should iterate.
| rule string, | ||
| values []string, | ||
| groupBy []string, | ||
| resourceId int32, |
There was a problem hiding this comment.
How can I get greater clarity on which of these are generated at the server/db layer and which are truly required by a User of tructl? Some of these like resourceVersion and fqn seem like they could always be ignored/defaulted "clientside"?
There was a problem hiding this comment.
Is looking at the .proto definitions the best place or is there somewhere it's clearer?
There was a problem hiding this comment.
I think so, but this is good feedback for the team and a issue is ideal.
There was a problem hiding this comment.
| Rule: GetAttributeRuleFromReadableString(rule), | ||
| Values: attrValues, | ||
| GroupBy: attrGroupBy, | ||
| Descriptor_: &commonv1.ResourceDescriptor{ |
There was a problem hiding this comment.
If some of these values are optional, should we be careful of zero values in the grpc calls (i.e. nil instead of an empty string slice) or is that handled elegantly in the service when lengths are zero? Looking at Definition.GroupBy which I know is optional and Definition.Descriptor_.Dependencies which I'm unsure.
There was a problem hiding this comment.
Update, looks like resource dependencies (Definition.Descriptor_.Dependencies) are also optional so we should clarify if we need to pass nil to the rpc or if an empty slice is okay.
| GroupBy: attrGroupBy, | ||
| Descriptor_: &commonv1.ResourceDescriptor{ | ||
| Type: commonv1.PolicyResourceType_POLICY_RESOURCE_TYPE_ATTRIBUTE_DEFINITION, | ||
| Id: resourceId, |
There was a problem hiding this comment.
Will the Definition.Descriptor_.Id always be the same as the top-level Id in the request? It seems like there's a (possibly buggy) behavior in the service-layer where passing a top-level Id that matches a real attribute Id and a different Definition.Descriptor_.Id will return a success response but no actual update/change applied on a subsequent get/list.
No description provided.