-
Notifications
You must be signed in to change notification settings - Fork 4.6k
delete command ready #18679
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
delete command ready #18679
Changes from 1 commit
9fa82df
cf2d3ef
5661d13
8a97d01
09076e6
c745c88
73119d9
9f00bb8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,173 @@ | ||
| // Copyright (c) HashiCorp, Inc. | ||
| // SPDX-License-Identifier: BUSL-1.1 | ||
|
|
||
| package delete | ||
|
|
||
| import ( | ||
| "errors" | ||
| "flag" | ||
| "fmt" | ||
|
|
||
| "github.com/mitchellh/cli" | ||
|
|
||
| "github.com/hashicorp/consul/agent/consul" | ||
| "github.com/hashicorp/consul/api" | ||
| "github.com/hashicorp/consul/command/flags" | ||
| "github.com/hashicorp/consul/command/helpers" | ||
| "github.com/hashicorp/consul/command/resource" | ||
| "github.com/hashicorp/consul/internal/resourcehcl" | ||
| ) | ||
|
|
||
| func New(ui cli.Ui) *cmd { | ||
| c := &cmd{UI: ui} | ||
| c.init() | ||
| return c | ||
| } | ||
|
|
||
| type cmd struct { | ||
| UI cli.Ui | ||
| flags *flag.FlagSet | ||
| http *flags.HTTPFlags | ||
| help string | ||
|
|
||
| filePath string | ||
| } | ||
|
|
||
| func (c *cmd) init() { | ||
| c.flags = flag.NewFlagSet("", flag.ContinueOnError) | ||
| c.http = &flags.HTTPFlags{} | ||
| c.flags.StringVar(&c.filePath, "f", "", "File path with resource definition") | ||
| flags.Merge(c.flags, c.http.ClientFlags()) | ||
| flags.Merge(c.flags, c.http.ServerFlags()) | ||
| flags.Merge(c.flags, c.http.MultiTenancyFlags()) | ||
| flags.Merge(c.flags, c.http.AddPeerName()) | ||
| c.help = flags.Usage(help, c.flags) | ||
| } | ||
|
|
||
| func (c *cmd) Run(args []string) int { | ||
| var gvk *api.GVK | ||
| var resourceName string | ||
| var opts *api.QueryOptions | ||
|
|
||
| if len(args) == 0 { | ||
| c.UI.Error("Please provide required arguments") | ||
| return 1 | ||
| } | ||
|
|
||
| if err := c.flags.Parse(args); err != nil { | ||
| if !errors.Is(err, flag.ErrHelp) { | ||
| c.UI.Error(fmt.Sprintf("Failed to parse args: %v", err)) | ||
| return 1 | ||
| } | ||
| } | ||
|
|
||
| if c.flags.Lookup("f").Value.String() != "" { | ||
| if c.filePath != "" { | ||
| data, err := helpers.LoadDataSourceNoRaw(c.filePath, nil) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you use the helper function
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yup, updated |
||
| if err != nil { | ||
| c.UI.Error(fmt.Sprintf("Failed to load data: %v", err)) | ||
| return 1 | ||
| } | ||
| parsedResource, err := resourcehcl.Unmarshal([]byte(data), consul.NewTypeRegistry()) | ||
| if err != nil { | ||
| c.UI.Error(fmt.Sprintf("Failed to decode resource from input file: %v", err)) | ||
| return 1 | ||
| } | ||
|
|
||
| gvk = &api.GVK{ | ||
| Group: parsedResource.Id.Type.GetGroup(), | ||
| Version: parsedResource.Id.Type.GetGroupVersion(), | ||
| Kind: parsedResource.Id.Type.GetKind(), | ||
| } | ||
| resourceName = parsedResource.Id.GetName() | ||
| opts = &api.QueryOptions{ | ||
| Namespace: parsedResource.Id.Tenancy.GetNamespace(), | ||
| Partition: parsedResource.Id.Tenancy.GetPartition(), | ||
| Peer: parsedResource.Id.Tenancy.GetPeerName(), | ||
| Token: c.http.Token(), | ||
| } | ||
| } else { | ||
| c.UI.Error(fmt.Sprintf("Please provide an input file with resource definition")) | ||
| return 1 | ||
| } | ||
| } else { | ||
| if len(args) < 2 { | ||
| c.UI.Error("Must specify two arguments: resource type and resource name") | ||
| return 1 | ||
| } | ||
| var err error | ||
| gvk, resourceName, err = resource.GetTypeAndResourceName(args) | ||
| if err != nil { | ||
| c.UI.Error(fmt.Sprintf("Your argument format is incorrect: %s", err)) | ||
| return 1 | ||
| } | ||
|
|
||
| inputArgs := args[2:] | ||
| if err := c.flags.Parse(inputArgs); err != nil { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, here we can use the helper function
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yup, updated |
||
| if errors.Is(err, flag.ErrHelp) { | ||
| return 0 | ||
| } | ||
| c.UI.Error(fmt.Sprintf("Failed to parse args: %v", err)) | ||
| return 1 | ||
| } | ||
| if c.filePath != "" { | ||
| c.UI.Error("You need to provide all information in the HCL file if provide its file path") | ||
| return 1 | ||
| } | ||
| opts = &api.QueryOptions{ | ||
| Namespace: c.http.Namespace(), | ||
| Partition: c.http.Partition(), | ||
| Peer: c.http.PeerName(), | ||
| Token: c.http.Token(), | ||
| } | ||
| } | ||
|
|
||
| client, err := c.http.APIClient() | ||
| if err != nil { | ||
| c.UI.Error(fmt.Sprintf("Error connect to Consul agent: %s", err)) | ||
| return 1 | ||
| } | ||
|
|
||
| if err := client.Resource().Delete(gvk, resourceName, opts); err != nil { | ||
| c.UI.Error(fmt.Sprintf("Error deleting resource %s.%s.%s/%s: %v", gvk.Group, gvk.Version, gvk.Kind, resourceName, err)) | ||
| return 1 | ||
| } | ||
|
|
||
| c.UI.Info(fmt.Sprintf("%s.%s.%s/%s deleted", gvk.Group, gvk.Version, gvk.Kind, resourceName)) | ||
| return 0 | ||
| } | ||
|
|
||
| func (c *cmd) Synopsis() string { | ||
| return synopsis | ||
| } | ||
|
|
||
| func (c *cmd) Help() string { | ||
| return flags.Usage(c.help, nil) | ||
| } | ||
|
|
||
| const synopsis = "Delete resource information" | ||
| const help = ` | ||
| Usage: You have two options to delete the resource specified by the given | ||
| type, name, partition, namespace and peer and outputs its JSON representation. | ||
|
|
||
| consul resource delete [type] [name] -partition=<default> -namespace=<default> -peer=<local> | ||
| consul resource delete -f [resource_file_path] | ||
|
|
||
| But you could only use one of the approaches. | ||
|
|
||
| Example: | ||
|
|
||
| $ consul resource delete catalog.v1alpha1.Service card-processor -partition=billing -namespace=payments -peer=eu | ||
| $ consul resource delete -f resource.hcl | ||
|
|
||
| In resource.hcl, it could be: | ||
| ID { | ||
| Type = gvk("catalog.v1alpha1.Service") | ||
| Name = "card-processor" | ||
| Tenancy { | ||
| Namespace = "payments" | ||
| Partition = "billing" | ||
| PeerName = "eu" | ||
| } | ||
| } | ||
| ` | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| // Copyright (c) HashiCorp, Inc. | ||
| // SPDX-License-Identifier: BUSL-1.1 | ||
| package delete | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/mitchellh/cli" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestResourceDeleteInvalidArgs(t *testing.T) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should add a test case where the resource type is in an invalid format
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup, as discussed above, we throw error if it's not the format of |
||
| t.Parallel() | ||
|
|
||
| type tc struct { | ||
| args []string | ||
| expectedCode int | ||
| expectedErrMsg string | ||
| } | ||
|
|
||
| cases := map[string]tc{ | ||
| "nil args": { | ||
| args: nil, | ||
| expectedCode: 1, | ||
| expectedErrMsg: "Please provide required arguments", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should have consistent error messaging across all the CLI commands. The messaging should start with See PR
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sounds good, I picked up the changes in that PR into this PR(only read.go and read_test.go) |
||
| }, | ||
| "empty args": { | ||
| args: []string{}, | ||
| expectedCode: 1, | ||
| expectedErrMsg: "Please provide required arguments", | ||
| }, | ||
| "missing file path": { | ||
| args: []string{"-f"}, | ||
| expectedCode: 1, | ||
| expectedErrMsg: "Please input file path", | ||
| }, | ||
| "provide type and name": { | ||
| args: []string{"a.b.c"}, | ||
| expectedCode: 1, | ||
| expectedErrMsg: "Must specify two arguments: resource type and resource name", | ||
| }, | ||
| "provide type and name with -f": { | ||
| args: []string{"a.b.c", "name", "-f", "test.hcl"}, | ||
| expectedCode: 1, | ||
| expectedErrMsg: "You need to provide all information in the HCL file if provide its file path", | ||
| }, | ||
| "provide type and name with -f and other flags": { | ||
| args: []string{"a.b.c", "name", "-f", "test.hcl", "-namespace", "default"}, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should just show a warning and not throw error in this case
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. makes sense, changed |
||
| expectedCode: 1, | ||
| expectedErrMsg: "You need to provide all information in the HCL file if provide its file path", | ||
| }, | ||
| "does not provide resource name after type": { | ||
| args: []string{"a.b.c", "-namespace", "default"}, | ||
| expectedCode: 1, | ||
| expectedErrMsg: "Must provide resource name right after type", | ||
| }, | ||
| } | ||
|
|
||
| for desc, tc := range cases { | ||
| t.Run(desc, func(t *testing.T) { | ||
| ui := cli.NewMockUi() | ||
| c := New(ui) | ||
|
|
||
| require.Equal(t, tc.expectedCode, c.Run(tc.args)) | ||
| require.NotEmpty(t, ui.ErrorWriter.String()) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added |
||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestResourceRead(t *testing.T) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The apply command is merged so you should be able to write test cases now 🙂
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup, added more test cases here(both read and delete) |
||
| // TODO: add read test after apply checked in | ||
| //if testing.Short() { | ||
| // t.Skip("too slow for testing.Short") | ||
| //} | ||
| // | ||
| //t.Parallel() | ||
| // | ||
| //a := agent.NewTestAgent(t, ``) | ||
| //defer a.Shutdown() | ||
| //client := a.Client() | ||
| // | ||
| //ui := cli.NewMockUi() | ||
| //c := New(ui) | ||
|
|
||
| //_, _, err := client.Resource().Apply() | ||
| //require.NoError(t, err) | ||
| // | ||
| //args := []string{} | ||
| // | ||
| //code := c.Run(args) | ||
| //require.Equal(t, 0, code) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| package resource | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "strings" | ||
|
|
||
| "github.com/hashicorp/consul/api" | ||
| ) | ||
|
|
||
| func GetTypeAndResourceName(args []string) (gvk *api.GVK, resourceName string, e error) { | ||
| // it has to be resource name after the type | ||
| if strings.HasPrefix(args[1], "-") { | ||
| return nil, "", fmt.Errorf("Must provide resource name right after type") | ||
| } | ||
|
|
||
| s := strings.Split(args[0], ".") | ||
| gvk = &api.GVK{ | ||
| Group: s[0], | ||
| Version: s[1], | ||
| Kind: s[2], | ||
| } | ||
|
Comment on lines
+52
to
+56
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will cause a panic if the resource type is not in Also, lets add a test case for it
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated, and added one more test case for it. |
||
|
|
||
| resourceName = args[1] | ||
| return | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need this check since we can rely on flags.args for required arguments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, removed this part.