[DATA-930] Add support for data deletion by filter to the CLI #1667
Conversation
viambot
added
the
safe to test
|
I still need to set up a local testing environment, but I wanted to send out for logic check |
| return errors.Errorf("type must be binary or tabular, got %s", c.String("type")) | ||
| } | ||
|
|
||
| filter := &datapb.Filter{} |
There was a problem hiding this comment.
Since the following logic on lines 876-933 is shared between this data and deletion commands, would suggest extracting into a helper method createDataFilter(filter *datapb.Filter) error
There was a problem hiding this comment.
great suggestion, done!
cli/cmd/main.go
Outdated
| @@ -296,6 +296,87 @@ func main() { | |||
| }, | |||
| Action: DataCommand, | |||
| }, | |||
| { | |||
| Name: "delete", | |||
There was a problem hiding this comment.
I'd lean towards calling this delete-data or deletedata but also nice to have single word commands. Getting another opinion @AaronCasas
There was a problem hiding this comment.
Hmm. I definitely think that this and the export command should be consistent in the sense that if this is data delete, that should be data export/download/whatever, and if this is just delete, that should just be export/download/whatever. I think I'd be partial towards the former (viam data <action) because I can definitely foresee a future where there are other delete operations we expose through the CLI (e.g. delete a robot).
Long winded way of saying: I'd vote data delete and changing data to data export or data download
There was a problem hiding this comment.
+1 to consistency and good suggestion, as solely passing --data isn't descriptive as we add more functionality to the CLI. I'd set this up with data as the main command and export and delete as the subcommands:
There was a problem hiding this comment.
+1 to using subcommands, especially since we have so many flags here that'll all be shared
There was a problem hiding this comment.
Great! Added as a subcommand
cli/cmd/main.go
Outdated
| dataType := c.String(dataFlagDataType) | ||
| switch dataType { | ||
| case dataTypeBinary: | ||
| if err := client.BinaryData(c.String(dataFlagDestination), filter); err != nil { |
There was a problem hiding this comment.
Ah, copy & paste mistake
cli/cmd/main.go
Outdated
| return err | ||
| } | ||
| case dataTypeTabular: | ||
| if err := client.BinaryData(c.String(dataFlagDestination), filter); err != nil { |
There was a problem hiding this comment.
Again, copy & paste mistake
cli/delete.go
Outdated
| ) | ||
|
|
||
| // DeleteBinaryData deletes binary data matching filter. | ||
| func (c *AppClient) DeleteBinaryData(dst string, filter *datapb.Filter) error { |
cli/delete.go
Outdated
| status := resp.GetResult().GetStatus() | ||
|
|
||
| switch status { | ||
| case datapb.Status_STATUS_PARTIAL_SUCCESS: |
There was a problem hiding this comment.
We can still have deleted some messages with a partial successes, so would still instead the deletedCount.
I think this section could be:
if status == datapb.Status_STATUS_PARTIAL_SUCCESS {
fmt.Fprint(c.c.App.Writer, "errors", resp.GetResult().GetMessage())
}
fmt.Fprintf(c.c.App.Writer, "deleted %d files", deletedCount)
return nil
Same for tabular
cli/delete.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func (c *AppClient) sendDeleteBinaryDataByFilterRequest(filter *datapb.Filter) (*datapb.DeleteBinaryDataByFilterResponse, error) { |
There was a problem hiding this comment.
Since these two helper functions aren't adding additional logic and are quite short as-is, worth keeping the requests directly inline with the code above. Gives the benefit of readability above without having to redirect to what the helper functions do.
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
|
|
||
| dataType := c.String(dataFlagDataType) | ||
| switch dataType { | ||
| case dataTypeBinary: |
There was a problem hiding this comment.
@AaronCasas I think this is showing up as something that is added in this PR since I re-arranged into the helper function. I believe this is fine, but can you just double check that this is as intended?
There was a problem hiding this comment.
Ah this is because your createDataFilter should stop at
if start != nil || end != nil {
filter.Interval = &datapb.CaptureInterval{
Start: start,
End: end,
}
}
But from copying over too much, it continues with client, err := rdkcli.NewAppClient and the rest of this client.BinaryData code. If you remove that, that'll fix the helper and should show a normal diff in GH.
There was a problem hiding this comment.
Ahh thank you! I missed it
cli/delete.go
Outdated
| if status == datapb.Status_STATUS_PARTIAL_SUCCESS { | ||
| fmt.Fprint(c.c.App.Writer, "received errors when deleting objects\n", resp.GetResult().GetMessage()) | ||
| } | ||
| fmt.Fprintf(c.c.App.Writer, "deleted %d files", deletedCount) |
viambot
added
safe to test
cli/cmd/main.go
Outdated
| @@ -712,6 +793,70 @@ func DataCommand(c *cli.Context) error { | |||
| } | |||
|
|
|||
| filter := &datapb.Filter{} | |||
| if err := createDataFilter(c, filter); err != nil { | |||
There was a problem hiding this comment.
I think in general it's better to return outputs* than modify inputs in place, so would suggest removing the filter parameter and just returning it instead:
filter, err := createDataFilter(c)
*A few reasons:
- If we modify an input, then we introduce a whole class of possible cases: what if the input is X/Y/Z/any variation here? This adds some complexity both because some of those inputs may be completely invalid (so require validation inside the function), and because it basically makes the "output" (in this case the implicit output being that modified input) a function of both the input state and the function implementation: instead of being able to treat the function like a black box that can be understood based solely on its name/signature/etc, a reader has to understand the initial input, the function, and how those two work together. Obviously this is super trivial in this case because the input is always the empty filter, but it leaves it open to these issues if that were to change in the future.
- The input is always the same (empty filter), so there's no need for the caller to need to provide it (and think about what they should provide/know it should be empty)
cli/cmd/main.go
Outdated
|
|
||
| // DeleteCommand runs the command for deleting data from the Viam cloud. | ||
| func DeleteCommand(c *cli.Context) error { | ||
| if c.String(dataFlagDataType) != dataTypeBinary && c.String(dataFlagDataType) != dataTypeTabular { |
There was a problem hiding this comment.
Can you pull this into some validateDataType that returns the below error since it's now being used in multiple places?
There was a problem hiding this comment.
Actually, with the switch below doing the same validation, I think this can be removed entirely. I think this error message is more informative though, so would suggest replacing that one with this one
There was a problem hiding this comment.
done, lmk if this implementation makes sense to you
cli/cmd/main.go
Outdated
| } | ||
|
|
||
| func createDataFilter(c *cli.Context, filter *datapb.Filter) error { | ||
| if c.String(dataFlagDataType) != dataTypeBinary && c.String(dataFlagDataType) != dataTypeTabular { |
There was a problem hiding this comment.
I think this validation makes sense to include somewhere outside of this function, since it isn't directly filter related (in the sense that I think it might be appropriate to include if it was validating something needed to build the filter, but the dataFlagDataType isn't otherwise used in here)
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
There was a problem hiding this comment.
Looks great, just one nit. Thanks for adding and reorganizing, excited about introducing subcommands 🚀🚀
Request that before submitting, you run local CLI tests that both deletion and data export work for both binary and tabular data. Thanks for updating the PR description to include how you ran deletion on binary!
cli/cmd/main.go
Outdated
| @@ -705,13 +792,75 @@ func main() { | |||
| } | |||
| } | |||
|
|
|||
| func validateDataType(c *cli.Context) (string, error) { | |||
There was a problem hiding this comment.
[supernit] I think since we're already switching between data types below, it makes sense to keep the validation directly inline like you previously had and return the "type must be binary or tabular, got:" in the default case. A few reasons:
- I think when the logic is simple enough, it's preferable to keep validations together with use cases for increased readability.
This differs from our filter validation in app where there's various combinations of invalid filters and more complicated query inference/population, but that's already created some issues of "how do I know in this function that I can access index 0? oh, I checked than len(arr) > 0 in the validate function" - As we add more API calls, they may be specific to binary (e.g. add/delete tags to existing images)
viambot
added
safe to test
cli/cmd/main.go
Outdated
| // DeleteCommand runs the command for deleting data from the Viam cloud. | ||
| func DeleteCommand(c *cli.Context) error { | ||
| dataType := c.String(dataFlagDataType) | ||
| if dataType != dataTypeBinary && dataType != dataTypeTabular { |
There was a problem hiding this comment.
I would still keep this as a switch case on dataType below in both command functions to avoid redundancy — we're doing a quick operation of creating a data filter and initializing an AppClient struct beforehand so ok wait to validate until after.
viambot
added
safe to test
|
This can be run locally by doing the following:
go build -o ~/go/bin/viam cli/cmd/main.goto build the cli toolviam data delete --data_type binary --org_ids [YOUR-ORG-ID] --start 2022-12-21T15:49:08Z