Add the ability to attach host IDs directly to targets#910
Conversation
github-actions
Bot
added
api
api/targets
core
core/db
core/gen
core/proto
core/servers
core/session
core/target
labels
|
Looks like the failing test: |
schristoff
left a comment
schristoff
left a comment
There was a problem hiding this comment.
These are more nitpicky suggestions than anything. Feel free to take them or close them out. Everything else looks okay though.
| return target, nil | ||
| } | ||
|
|
||
| func (c *Client) AddHosts(ctx context.Context, targetId string, version uint32, hostIds []string, opt ...Option) (*TargetUpdateResult, error) { |
There was a problem hiding this comment.
I feel like a note on what is happening here would be useful, especially since it is public. :) But not needed.
There was a problem hiding this comment.
Yeah that's fair, although given it's automatically generated there's some work involved to have anything meaningful added.
| if api.AsServerError(existingErr) != nil { | ||
| return nil, fmt.Errorf("error from controller when performing initial check-and-set read: %w", existingErr) | ||
| } | ||
| return nil, fmt.Errorf("error performing initial check-and-set read: %w", existingErr) |
There was a problem hiding this comment.
Why do some errors use fmt.Errorf and some errors use errors.?
There was a problem hiding this comment.
Generally if there are no formatting directives errors.New is used. Within internal there is a separate errors package that is used for some types of errors.
There was a problem hiding this comment.
Ah, previously I would wrap something like fmt.Sprintf in errors.New or errors.WithMsg()
Gotchya though
|
|
||
| opts, apiOpts := getOpts(opt...) | ||
|
|
||
| if version == 0 { |
There was a problem hiding this comment.
Would it be possible to take some of these validation steps out of Add, Set, and Remove hosts into their own functions? It would make the functions quite a bit smaller and reduce the repeating code.
There was a problem hiding this comment.
Yeah it might be. Since it's all autogenerated repeating code is really "code in one place that gets slotted in" but from a cleanliness perspective it might be nice.
| helpStr = base.WrapForHelpText([]string{ | ||
| "Usage: boundary target set-hosts [options] [args]", | ||
| "", | ||
| " This command allows setting the complete set of host resources on a target resource. Example:", |
There was a problem hiding this comment.
This could just be my confusion, but it may be good to add how this is different than add?
There was a problem hiding this comment.
Yeah it's subtle, but see how it says "setting the complete set of host resources" vs. "adding host resources".
Co-authored-by: hashicorp-tsccr[bot] <hashicorp-tsccr[bot]@users.noreply.github.com>
4 participants
No description provided.