Skip to content
This repository has been archived by the owner on Dec 7, 2023. It is now read-only.

rm: Add --config flag #525

Merged
merged 1 commit into from
Feb 26, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/ignite/cmd/vmcmd/rm.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ func NewCmdRm(out io.Writer) *cobra.Command {
separated by spaces. The force flag (-f, --force) kills running
VMs before removal instead of throwing an error.
`),
Args: cobra.MinimumNArgs(1),
Run: func(cmd *cobra.Command, args []string) {
cmdutil.CheckErr(func() error {
ro, err := rf.NewRmOptions(args)
Expand All @@ -42,4 +41,5 @@ func NewCmdRm(out io.Writer) *cobra.Command {

func addRmFlags(fs *pflag.FlagSet, rf *run.RmFlags) {
cmdutil.AddForceFlag(fs, &rf.Force)
cmdutil.AddConfigFlag(fs, &rf.ConfigFile)
}
38 changes: 34 additions & 4 deletions cmd/ignite/run/rm.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,25 +4,55 @@ import (
"fmt"

api "github.com/weaveworks/ignite/pkg/apis/ignite"
"github.com/weaveworks/ignite/pkg/apis/ignite/scheme"
"github.com/weaveworks/ignite/pkg/operations"
"github.com/weaveworks/ignite/pkg/providers"
)

// RmFlags contains the flags supported by the remove command.
type RmFlags struct {
Force bool
Force bool
ConfigFile string
}

type rmOptions struct {
*RmFlags
vms []*api.VM
}

func (rf *RmFlags) NewRmOptions(vmMatches []string) (ro *rmOptions, err error) {
ro = &rmOptions{RmFlags: rf}
// NewRmOptions creates and returns rmOptions with all the flags and VMs to be
// removed.
func (rf *RmFlags) NewRmOptions(vmMatches []string) (*rmOptions, error) {
ro := &rmOptions{RmFlags: rf}

// If config file is provided, use it to find the VM to be removed.
if len(rf.ConfigFile) != 0 {
Copy link
Member

Choose a reason for hiding this comment

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

if it's provided, you mean if len(rf.ConfigFile) > 0 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right, I was checking if config file path isn't empty. In the lines of https://github.com/weaveworks/ignite/blob/v0.6.3/cmd/ignite/run/create.go#L73
Could have compared with "" 🙂
Should I still change it?

Copy link
Member

Choose a reason for hiding this comment

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

np.

if len(vmMatches) > 0 {
return ro, fmt.Errorf("cannot use both config flag and vm argument")
}

vm := &api.VM{}
if err := scheme.Serializer.DecodeFileInto(rf.ConfigFile, vm); err != nil {
return ro, err
}
// Name or UID must be provided in the config file.
if len(vm.Name) == 0 && len(vm.UID) == 0 {
return ro, fmt.Errorf("API resource config must have Name or UID")
}
ro.vms = []*api.VM{vm}
return ro, nil
}

// Use vm args to find the VMs to be removed.
if len(vmMatches) < 1 {
Copy link
Member

Choose a reason for hiding this comment

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

I see. You removed minimumNArgs(1) above because you added a check here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's right. When a config file is provided, a vm argument is not necessary.

return ro, fmt.Errorf("need at least one vm identifier as argument")
}
var err error
ro.vms, err = getVMsForMatches(vmMatches)
return
return ro, err
}

// Rm removes VMs based on rmOptions.
func Rm(ro *rmOptions) error {
for _, vm := range ro.vms {
// If the VM is running, but we haven't enabled force-mode, return an error
Expand Down
162 changes: 162 additions & 0 deletions cmd/ignite/run/rm_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,162 @@
package run

import (
"io/ioutil"
"os"
"path/filepath"
"testing"

"github.com/weaveworks/gitops-toolkit/pkg/runtime"
"github.com/weaveworks/gitops-toolkit/pkg/storage"
"github.com/weaveworks/gitops-toolkit/pkg/storage/cache"

api "github.com/weaveworks/ignite/pkg/apis/ignite"
"github.com/weaveworks/ignite/pkg/apis/ignite/scheme"
meta "github.com/weaveworks/ignite/pkg/apis/meta/v1alpha1"
"github.com/weaveworks/ignite/pkg/client"
"github.com/weaveworks/ignite/pkg/providers"
"github.com/weaveworks/ignite/pkg/util"
)

func TestNewRmOptions(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

This test LGTM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks 🙂

testdataDir := "testdata"

cases := []struct {
name string
existingVMs []string
rmFlags *RmFlags
vmMatches []string // argument of NewRmOptions()
wantMatches []string
err bool
}{
{
name: "rm with vm arg",
existingVMs: []string{"myvm1", "myvm2", "myvm3"},
rmFlags: &RmFlags{},
vmMatches: []string{"myvm2"},
wantMatches: []string{"myvm2"},
},
{
name: "rm with multiple vm args",
existingVMs: []string{"myvm1", "myvm2", "myvm3"},
rmFlags: &RmFlags{},
vmMatches: []string{"myvm2", "myvm3"},
wantMatches: []string{"myvm2", "myvm3"},
},
{
name: "error rm non-existing vm",
existingVMs: []string{"myvm1", "myvm2", "myvm3"},
rmFlags: &RmFlags{},
vmMatches: []string{"myvm4"},
err: true,
},
{
name: "error rm without any args or config flag",
existingVMs: []string{"myvm1", "myvm2", "myvm3"},
rmFlags: &RmFlags{},
err: true,
},
{
name: "error rm with vm arg and config flag",
existingVMs: []string{"myvm1"},
rmFlags: &RmFlags{ConfigFile: "foo.yaml"},
vmMatches: []string{"myvm1"},
err: true,
},
{
name: "rm with config file",
existingVMs: []string{"myvm1", "myvm2", "myvm3"},
rmFlags: &RmFlags{ConfigFile: filepath.Join(testdataDir, "input/rm-vm1.yaml")},
wantMatches: []string{"myvm2"},
},
{
name: "error rm config name and uid missing",
existingVMs: []string{"myvm1"},
rmFlags: &RmFlags{ConfigFile: filepath.Join(testdataDir, "input/rm-no-name-uid.yaml")},
err: true,
},
}

for _, rt := range cases {
t.Run(rt.name, func(t *testing.T) {
// Create storage.
dir, err := ioutil.TempDir("", "ignite")
if err != nil {
t.Fatalf("failed to create storage for ignite: %v", err)
}
defer os.RemoveAll(dir)

storage := cache.NewCache(
storage.NewGenericStorage(
storage.NewGenericRawStorage(dir), scheme.Serializer))

// Create ignite client with the created storage.
ic := client.NewClient(storage)

// Create the existing VMs.
for _, objectName := range rt.existingVMs {
vm := &api.VM{}
vm.SetName(objectName)

// Set UID.
uid, err := util.NewUID()
if err != nil {
t.Errorf("failed to generate new UID: %v", err)
}
vm.SetUID(runtime.UID(uid))

// Set VM image.
ociRef, err := meta.NewOCIImageRef("foo/bar:latest")
if err != nil {
t.Errorf("failed to create new image reference: %v", err)
}
img := &api.Image{
Spec: api.ImageSpec{
OCI: ociRef,
},
}
vm.SetImage(img)

// Set Kernel image.
ociRefKernel, err := meta.NewOCIImageRef("foo/bar:latest")
if err != nil {
t.Errorf("failed to create new image reference: %v", err)
}
kernel := &api.Kernel{
Spec: api.KernelSpec{
OCI: ociRefKernel,
},
}
vm.SetKernel(kernel)

// Save object.
if err := ic.VMs().Set(vm); err != nil {
t.Errorf("failed to store VM object: %v", err)
}
}

// Set provider client used in remove to find VM matches.
providers.Client = ic

// Create new rm options using the rmFlags and vmMatches.
ro, err := rt.rmFlags.NewRmOptions(rt.vmMatches)
if (err != nil) != rt.err {
t.Fatalf("expected error %t, actual: %v", rt.err, err)
}

// Check if the wanted VMs are in the matched VMs list.
for _, wantVM := range rt.wantMatches {
found := false
for _, vm := range ro.vms {
if vm.Name == wantVM {
found = true
break
}
}
if !found {
t.Errorf("expected vm %q to be in remove vm list", wantVM)
}
}
})
}
}
5 changes: 5 additions & 0 deletions cmd/ignite/run/testdata/input/rm-no-name-uid.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
apiVersion: ignite.weave.works/v1alpha2
kind: VM
metadata:
name: ""
uid: ""
5 changes: 5 additions & 0 deletions cmd/ignite/run/testdata/input/rm-vm1.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
apiVersion: ignite.weave.works/v1alpha2
kind: VM
metadata:
name: myvm2
uid: 599615df99804ae8
5 changes: 3 additions & 2 deletions docs/cli/ignite/ignite_rm.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,9 @@ ignite rm <vm>... [flags]
### Options

```
-f, --force Force this operation. Warning, use of this mode may have unintended consequences.
-h, --help help for rm
--config string Specify a path to a file with the API resources you want to pass
-f, --force Force this operation. Warning, use of this mode may have unintended consequences.
-h, --help help for rm
```

### Options inherited from parent commands
Expand Down
5 changes: 3 additions & 2 deletions docs/cli/ignite/ignite_vm_rm.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,9 @@ ignite vm rm <vm>... [flags]
### Options

```
-f, --force Force this operation. Warning, use of this mode may have unintended consequences.
-h, --help help for rm
--config string Specify a path to a file with the API resources you want to pass
-f, --force Force this operation. Warning, use of this mode may have unintended consequences.
-h, --help help for rm
```

### Options inherited from parent commands
Expand Down