[DATA-850] Support tags in CLI command #1714
Conversation
viambot
added
the
safe to test
viambot
added
safe to test
cli/cmd/main.go
Outdated
| Tags: c.StringSlice(dataFlagTags), | ||
| } | ||
| } | ||
| if c.String(dataFlagTaggedUntagged) != "" { |
There was a problem hiding this comment.
Thinking through how to incorporate tags and made sense to me have separate use cases for 1) filtering based on matching tags and then grabbing either 2) all tagged data or 3) all untagged data.
Very open to alternative ways of going about this
cli/cmd/main.go
Outdated
| } | ||
| case "untagged": | ||
| filter.TagsFilter = &datapb.TagsFilter{ | ||
| Type: datapb.TagsFilterType_TAGS_FILTER_TYPE_TAGGED, |
There was a problem hiding this comment.
Looks like this should be untagged
There was a problem hiding this comment.
thanks for the catch
cli/cmd/main.go
Outdated
| &cli.StringSliceFlag{ | ||
| Name: dataFlagTags, | ||
| Required: false, | ||
| Usage: "indicates filtering based on matching tags", |
There was a problem hiding this comment.
I think this usage message should indicate that it's going to return all data matching any of the passed tags vs those matching all of them. I think the default there is non-obvious, so good to be explicit
cli/cmd/main.go
Outdated
| @@ -303,6 +305,16 @@ func main() { | |||
| Required: false, | |||
| Usage: "ISO-8601 timestamp indicating the end of the interval filter", | |||
| }, | |||
| &cli.StringFlag{ | |||
There was a problem hiding this comment.
I feel like it's a little weird for this to be split across two commands, since both are for the end user use case of "give me data with tags <anything/nothing/specific-things>".
I think it might make sense for them to just be one command and "tagged" and "untagged" to basically be keywords to the dataFlagTags command, i.e. if they pass tags=tagged it would be equivalent to to dataFlagTaggedUntagged=tagged currently. Open to other opinions on how best to combine them, but I do think they should probably be combined
There was a problem hiding this comment.
I think that's a reasonable solution, agree that want to keep it simple in the CLI versus the explicit nested tags/types that we expose via the API.
So they can pass tags=tagged, tags=untagged, or tags=square,triangle,circle. Would explain that in the usage text, e.g. "indicates filtering based on data with tags. accepts tagged for all tagged data, untagged for all untagged data, or a list of tags for all data matching any of the tags"
If they're specifically only trying to filter on a single tag that's named "tagged" or "untagged" this fails but don't think we need to worry about that :)
There was a problem hiding this comment.
done!
if they are trying to filter on "tagged" or "untagged",
think that tags=,tagged would do the trick
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
cli/cmd/main.go
Outdated
| if c.String(dataFlagTaggedUntagged) != "" { | ||
| switch c.String(dataFlagTaggedUntagged) { | ||
| tagsLen := len(c.StringSlice(dataFlagTags)) | ||
| switch c.StringSlice(dataFlagTags)[0] { |
There was a problem hiding this comment.
Does this NPE if passed an empty list (if that's possible through the cli even)? If so, should probably add a len==0 check first
There was a problem hiding this comment.
StringSlice() returns nil if not set, but imo for readability/usability, this and the above two checks can be if len(c.StringSlice(dataFlagTags)) > 0
In this case, makes it clear that it's valid to grab from the first index
There was a problem hiding this comment.
What if it's set but empty? Like --org_ids=a,b --tags= --robot_name=name. Or is that just interpreted as not being set?
There was a problem hiding this comment.
Just ran it locally, it's interpreted as an array of len 1 with an empty string.
cli/cmd/main.go
Outdated
| case "tagged": | ||
| filter.TagsFilter = &datapb.TagsFilter{ | ||
| Type: datapb.TagsFilterType_TAGS_FILTER_TYPE_TAGGED, | ||
| if tagsLen == 1 { |
There was a problem hiding this comment.
What happens if tagsLen != 1? It looks like it might silently fail or behave in unexpected ways (however our backend responds if no filter is passed)
cli/cmd/main.go
Outdated
| &cli.StringSliceFlag{ | ||
| Name: dataFlagTags, | ||
| Required: false, | ||
| Usage: "indicates filtering based on data with tags. accepts tagged for all tagged data, untagged for all untagged data, or a list of tags for all data matching any of the tags", |
There was a problem hiding this comment.
[nit] to shorten this up, can just be "tags filter. accepts ..."
cli/cmd/main.go
Outdated
| if c.String(dataFlagTaggedUntagged) != "" { | ||
| switch c.String(dataFlagTaggedUntagged) { | ||
| tagsLen := len(c.StringSlice(dataFlagTags)) | ||
| switch c.StringSlice(dataFlagTags)[0] { |
There was a problem hiding this comment.
StringSlice() returns nil if not set, but imo for readability/usability, this and the above two checks can be if len(c.StringSlice(dataFlagTags)) > 0
In this case, makes it clear that it's valid to grab from the first index
viambot
added
safe to test
cli/cmd/main.go
Outdated
| switch c.StringSlice(dataFlagTags)[0] { | ||
| case "tagged": | ||
| if tagsLen == 1 { | ||
| if len(c.StringSlice(dataFlagTags)) == 1 { |
There was a problem hiding this comment.
I think this can be simplified with:
if len == 1 && [0] == tagged {
// all tagged case
} else if len == 1 && [0] == untagged {
// all untagged case
} else {
// or case (the current else case)
}
There was a problem hiding this comment.
yes you're right! ty
viambot
added
safe to test
viambot
added
safe to test
|
No description provided.