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

Conversation

darkowlzz
Copy link
Contributor

Adds --config flag in rm command to accept VM removal using the same
config file that was used to create the VM. VM name or UID is required
when passing a config file.

Adds tests to verify the changes.

Adds --config flag in rm command to accept VM removal using the same
config file that was used to create the VM. VM name or UID is required
when passing a config file.

Adds tests to verify the changes.
@chanwit chanwit requested review from chanwit and removed request for twelho February 17, 2020 18:07
}

// 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.

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.

"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 🙂

@chanwit
Copy link
Member

chanwit commented Feb 17, 2020

Thank you @darkowlzz for this PR.
There's a small nit and I'm also not sure why the test is failing. It might be unrelated.

@chanwit
Copy link
Member

chanwit commented Feb 25, 2020

@darkowlzz @stealthybox
The dependency changed and it failed the CI. It needs make tidy a bit.
But I'll be going to merge this if you guys are OK with it?

@darkowlzz
Copy link
Contributor Author

In the last meeting, we discussed about the CI failures due to dependency update check. We talked about removing the dependency update check. I'm okay with merging this and removing the check separately.

@chanwit
Copy link
Member

chanwit commented Feb 26, 2020

LGTM then :-)

@chanwit chanwit merged commit b76b454 into weaveworks:master Feb 26, 2020
@darkowlzz darkowlzz deleted the rm-config-flag branch February 26, 2020 08:52
@luxas luxas added this to the v0.7.0 milestone Jun 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants