Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat(cli): add/set/remove worker tags #2266

Merged
merged 17 commits into from
Aug 26, 2022
Merged

feat(cli): add/set/remove worker tags #2266

merged 17 commits into from
Aug 26, 2022

Conversation

A-440Hz
Copy link
Contributor

@A-440Hz A-440Hz commented Jul 19, 2022

example commands:

adding key with multiple values, adding multiple keys:

bin/boundary workers add-worker-tags -id w_1234567890 -tag key=val1,val2 -tag key2=val

Worker information:
Active Connection Count: 0
Address: 127.0.0.1:9202
Created Time: Mon, 25 Jul 2022 10:03:39 PDT
ID: w_1234567890
Last Status Time: 2022-07-25 19:41:09.485499 +0000 UTC
Type: pki
Updated Time: Mon, 25 Jul 2022 12:41:09 PDT
Version: 2

Scope:
ID: global
Name: global
Type: global

Tags:
Configuration:
type: ["dev" "local"]
Api:
key: ["val1" "val2"]
key2: ["val"]
Canonical:
key: ["val1" "val2"]
key2: ["val"]
type: ["dev" "local"]

removing a single tag:

bin/boundary workers remove-worker-tags -id w_1234567890 -tag key=val1

Tags:
Configuration:
type: ["dev" "local"]
Api:
key: ["val2"]
key2: ["val"]
Canonical:
key: ["val2"]
key2: ["val"]
type: ["dev" "local"]

setting all tags to nil:

bin/boundary workers set-worker-tags -id w_1234567890 -tag null

Tags:
Configuration:
type: ["dev" "local"]
Canonical:
type: ["dev" "local"]

@A-440Hz
Copy link
Contributor Author

A-440Hz commented Jul 19, 2022

just the string parsing/initial flag stuff for now

@irenarindos
Copy link
Collaborator

irenarindos commented Jul 19, 2022

Can you add the new commands to internal/cmd/commands.go?

And the following in internal/cmd/gencli/input.go
VersionedActions: []string{"update", "add-worker-tags", "set-worker-tags", "remove-worker-tags"},

Copy link
Collaborator

@irenarindos irenarindos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a suggestion around testing some error cases...

internal/tests/api/workers/worker_test.go Show resolved Hide resolved
internal/tests/api/workers/worker_test.go Show resolved Hide resolved
@irenarindos
Copy link
Collaborator

Looks like we're going to hold off on merging this until @jefferai is back and can take a look

internal/cmd/base/flags.go Outdated Show resolved Hide resolved
internal/cmd/base/flags.go Outdated Show resolved Hide resolved
internal/cmd/base/flags.go Show resolved Hide resolved

func extraFlagsHandlingFuncImpl(c *Command, _ *base.FlagSets, opts *[]workers.Option) bool {
switch c.Func {
case "add-worker-tags", "remove-worker-tags":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is missing a set-worker-tags case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want an empty -tag entry to convert to nil for set-worker-tags? I thought it was pretty nifty when boundary set-worker-tags -id w_idxxxxxxxx cleared the tags

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a set-worker-tags case with No tag supplied via -tag. Please use "-tag null" to clear all tags as the UI error when there is no -tag input

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually yeah I'm not sure what to do here since if I check whether c.FlagTags is empty, it overrides the "null" case--where I set target to nil for a stringSliceMapValue in flags.go--and returns an error.

I don't want to set a token entry for the map[string][]string to represent that its value should be nil, since that limits a valid key from use (even if it's a mapping like null=true). I don't like the idea of using an invalid key with commas like some form of ,,null,, since I think that's a bit confusing and unintuitive.

The most clear way forward might be creating another/using another existing bool to distinguish when the user inputs a "null" entry for set-worker-tags..

However, do I have to put a set-worker-tags case here? Is it alright for the user to be able to null the entry via boundary set-worker-tags -id w_idxxxxxxxx and boundary set-worker-tags -id w_idxxxxxxxx -tag null?

@@ -546,13 +546,19 @@ func Parse(d string) (*Config, error) {
if !strutil.Printable(k) {
return nil, fmt.Errorf("Tag key %q contains non-printable characters", k)
}
if strings.Contains(k, ",") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that this is necessarily wrong, but what's the reason for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since no commas are allowed thru the CLI entry, this applies the same standards to tag entries from the config file. Otherwise you can input keys such as "k3,k4" if you input them this way

tags {
key = ["val1", "val2"],
key2 = ["val1", "val2"],
"k3,k4" = ["vvv"],
}

@A-440Hz A-440Hz added this to the 0.10.3 milestone Aug 25, 2022
@A-440Hz A-440Hz requested a review from jefferai August 25, 2022 23:12
@A-440Hz A-440Hz merged commit 17ddc30 into main Aug 26, 2022
@A-440Hz A-440Hz deleted the hz-modify-api-tags-3 branch August 26, 2022 20:37
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants