Skip to content

Prevent vic-machine from deleting non cVMs#6816

Merged
andrewtchin merged 3 commits intomasterfrom
chin/6603/vic-machine-overzealous
Nov 21, 2017
Merged

Prevent vic-machine from deleting non cVMs#6816
andrewtchin merged 3 commits intomasterfrom
chin/6603/vic-machine-overzealous

Conversation

@andrewtchin
Copy link
Contributor

Fixes #6603
Replacement PR for #6679 to get the build to build things

@andrewtchin andrewtchin changed the title [full ci] Prevent vic-machine from deleting non cVMs [specific ci=Group6-VIC-Machine] Prevent vic-machine from deleting non cVMs Nov 21, 2017
@andrewtchin andrewtchin force-pushed the chin/6603/vic-machine-overzealous branch from df029c1 to bb50104 Compare November 21, 2017 15:48
}

func (d *Dispatcher) isContainerVM(vm *vm.VirtualMachine) (bool, error) {
if vm == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible that anybody pass a nil value here? Asking, because of excessive check is actually bad for readability and maintainability.

@andrewtchin andrewtchin requested a review from hickeng November 21, 2017 20:25
Copy link
Contributor

@mdharamadas1 mdharamadas1 left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@mhagen-vmware mhagen-vmware left a comment

Choose a reason for hiding this comment

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

You have ha-datacenter riddled throughout here, I think in most cases govc will auto-find without that?

Should Contain ${output} Completed successfully

# Verify VM exists
${rc} ${output}= Run And Return Rc And Output govc ls /ha-datacenter/vm/${dummyvm}
Copy link
Contributor

Choose a reason for hiding this comment

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

that is going to break on vc... I think. I think you don't need ha-datacenter for the ls call, just let it assume path and do govc ls vm/${...}

Copy link
Contributor

Choose a reason for hiding this comment

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

otherwise, we should use some combination of %{TEST_DATACENTER} instead

Should Be Equal As Integers ${rc} 0

# Verify VM exists
${rc} ${output}= Run And Return Rc And Output govc ls /ha-datacenter/vm/${dummyvm}
Copy link
Contributor

Choose a reason for hiding this comment

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

same as below

Copy link
Contributor

@mhagen-vmware mhagen-vmware left a comment

Choose a reason for hiding this comment

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

lgtm

@andrewtchin andrewtchin changed the title [specific ci=Group6-VIC-Machine] Prevent vic-machine from deleting non cVMs Prevent vic-machine from deleting non cVMs Nov 21, 2017
@andrewtchin
Copy link
Contributor Author

Group6-VIC-Machine/6-03-Delete.robot passed locally on ESX and nimbus VC
https://ci.vcna.io/vmware/vic/14966 Group6 passed except for a flaky test

@andrewtchin andrewtchin merged commit 42fbe58 into master Nov 21, 2017
@andrewtchin andrewtchin deleted the chin/6603/vic-machine-overzealous branch November 21, 2017 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants