Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Reimplemented a common delete tink-cli command #433

Merged
merged 4 commits into from
Feb 9, 2021

Conversation

gauravgahlot
Copy link
Contributor

@gauravgahlot gauravgahlot commented Feb 4, 2021

Description

# Delete template resource (success)
tink template delete 8ae1cc24-6a9c-11eb-a0fc-0242ac120005
Deleted 8ae1cc24-6a9c-11eb-a0fc-0242ac120005
                                                                            
# Delete template resource (not found)                                     
tink template delete 8ae1cc24-6a9c-11eb-a0fc-0242ac120005
Error   8ae1cc24-6a9c-11eb-a0fc-0242ac120005    not found
                                                                     
# Delete template resources (one not found)                                 
tink template delete 8ae1cc24-6a9c-11eb-a0fc-0242ac120005 e4115856-4358-429d-a8f6-9e1b7d794b72
Deleted 8ae1cc24-6a9c-11eb-a0fc-0242ac120005                                               
Error   e4115856-4358-429d-a8f6-9e1b7d794b72    not found                                  
                                                                                            
# Delete resources and exract resource ID with awk                                         
tink template delete 8ae1cc24-6a9c-11eb-a0fc-0242ac120005 e4115856-4358-429d-a8f6-9e1b7d794b72 | awk {print $2} > result
cat result                                                                                                           
8ae1cc24-6a9c-11eb-a0fc-0242ac120005                                                                                 
e4115856-4358-429d-a8f6-9e1b7d794b72         

Why is this needed

Fixes: #425

How Has This Been Tested?

  • Manual testing
  • Unit tests

Checklist:

I have:

  • updated the documentation and/or roadmap (if required)
  • added unit or e2e tests
  • provided instructions on how to upgrade

Signed-off-by: Gaurav Gahlot <[email protected]>
@gauravgahlot gauravgahlot added kind/feature Categorizes issue or PR as related to a new feature. area/tink-cli labels Feb 4, 2021
@gauravgahlot gauravgahlot self-assigned this Feb 4, 2021
@gauravgahlot gauravgahlot added the needs-tests Needs supporting unit tests label Feb 4, 2021
Copy link
Contributor

@gianarb gianarb left a comment

Choose a reason for hiding this comment

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

I like where this is going!

const longDescr = `Deletes one or many resources and prints the status of
the deleted resource.`

const exampleDescr = `# Delete template resource(s)
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you should add a reference to deleting more than one ID.

tink hardware delete [id1] [id2][id3]

You can place an example of the output as well.

As part of the long description you can add an explanation about the output format and how you can pipe it with other commands like awk.

_, err := opt.DeleteByID(cmd.Context(), opt.fullClient, requestedID)
if err != nil {
if s, ok := status.FromError(err); ok && s.Code() == codes.NotFound {
fmt.Printf("Error\t%s\tnot found\n", requestedID)
Copy link
Contributor

Choose a reason for hiding this comment

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

You have to print to stderr cmd.Stderr

return err
}
}
fmt.Printf("Deleted\t%s\n", requestedID)
Copy link
Contributor

Choose a reason for hiding this comment

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

Print to cmd.Stdout

@codecov
Copy link

codecov bot commented Feb 9, 2021

Codecov Report

Merging #433 (61fe4ff) into master (5dce031) will increase coverage by 0.37%.
The diff coverage is 39.68%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #433      +/-   ##
==========================================
+ Coverage   34.34%   34.71%   +0.37%     
==========================================
  Files          46       47       +1     
  Lines        2865     2892      +27     
==========================================
+ Hits          984     1004      +20     
- Misses       1794     1796       +2     
- Partials       87       92       +5     
Impacted Files Coverage Δ
cmd/tink-cli/cmd/hardware/delete.go 0.00% <0.00%> (ø)
cmd/tink-cli/cmd/template.go 0.00% <0.00%> (ø)
cmd/tink-cli/cmd/template/delete.go 0.00% <0.00%> (ø)
cmd/tink-cli/cmd/workflow.go 0.00% <0.00%> (ø)
db/hardware.go 76.57% <33.33%> (-1.41%) ⬇️
db/template.go 47.72% <33.33%> (-0.74%) ⬇️
db/workflow.go 29.06% <33.33%> (-0.12%) ⬇️
cmd/tink-cli/cmd/delete/delete.go 51.28% <51.28%> (ø)
cmd/tink-cli/cmd/hardware.go 75.00% <100.00%> (ø)
cmd/tink-cli/cmd/template/get.go 42.85% <100.00%> (ø)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5dce031...86d5f24. Read the comment docs.

@gianarb
Copy link
Contributor

gianarb commented Feb 9, 2021

Awesome work!!! Thanks

Copy link
Contributor

@gianarb gianarb left a comment

Choose a reason for hiding this comment

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

I forget :) the work is great but do you mind squashing commits?! Thanks

@gauravgahlot gauravgahlot added the ready-to-merge Signal to Mergify to merge the PR. label Feb 9, 2021
@mergify mergify bot merged commit 5752ec8 into tinkerbell:master Feb 9, 2021
@gauravgahlot gauravgahlot deleted the new-delete branch February 9, 2021 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/tink-cli kind/feature Categorizes issue or PR as related to a new feature. ready-to-merge Signal to Mergify to merge the PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reimplement a common delete tink-cli command
2 participants