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

machines: Delete #7113

Merged
merged 1 commit into from Jul 3, 2017

Conversation

Projects
None yet
5 participants
@mvollmer
Member

mvollmer commented Jun 26, 2017

  • integration test

@mvollmer mvollmer changed the title from WIP - machines: Delete to machines: Delete Jun 28, 2017

@stefwalter

This comment has been minimized.

Show comment
Hide comment
@stefwalter

stefwalter Jun 29, 2017

Contributor

This needs an update now that the #7118 has changed.

# testDelete (check_machines.TestMachines)
#
not ok 13 testDelete (check_machines.TestMachines) duration: 9s
Traceback (most recent call last):
  File "/build/cockpit/bots/../test/verify/check-machines", line 273, in testDelete
    state = self.envSetup('vnc')
AttributeError: 'TestMachines' object has no attribute 'envSetup'
Journal extracted to TestMachines-testDelete-10.111.116.113-FAIL.log
Contributor

stefwalter commented Jun 29, 2017

This needs an update now that the #7118 has changed.

# testDelete (check_machines.TestMachines)
#
not ok 13 testDelete (check_machines.TestMachines) duration: 9s
Traceback (most recent call last):
  File "/build/cockpit/bots/../test/verify/check-machines", line 273, in testDelete
    state = self.envSetup('vnc')
AttributeError: 'TestMachines' object has no attribute 'envSetup'
Journal extracted to TestMachines-testDelete-10.111.116.113-FAIL.log
@martinpitt

Looks fine by and large, I just found one i18n issue and a bunch of nitpicks (feel free to ignore those). Can you please add a screenshot? @garrett , do you mind a quick review of this?

Show outdated Hide outdated pkg/machines/README.md
Show outdated Hide outdated pkg/machines/components/deleteDialog.jsx
Show outdated Hide outdated pkg/machines/components/deleteDialog.jsx
Show outdated Hide outdated pkg/machines/components/deleteDialog.jsx
Show outdated Hide outdated pkg/machines/components/deleteDialog.jsx
Show outdated Hide outdated pkg/machines/hostvmslist.jsx
img1 = "/var/lib/libvirt/images/{0}.img".format(name)
img2 = "/var/lib/libvirt/images/{0}-2.img".format(name)
self.startVm(name, console='vnc')

This comment has been minimized.

@martinpitt

martinpitt Jun 29, 2017

Member

Not that it matters much, but why is specifying "vnc" explicitly important here?

@martinpitt

martinpitt Jun 29, 2017

Member

Not that it matters much, but why is specifying "vnc" explicitly important here?

This comment has been minimized.

@mvollmer

mvollmer Jun 29, 2017

Member

No, this is copied from the other tests.

@mvollmer

mvollmer Jun 29, 2017

Member

No, this is copied from the other tests.

b.wait_not_present("#vm-{0}-row".format(name))
m.execute("! test -f {0}".format(img1))
m.execute("! test -f {0}".format(img2))

This comment has been minimized.

@martinpitt

martinpitt Jun 29, 2017

Member

Might be worth asserting that the VM is also gone from virsh list --all

@martinpitt

martinpitt Jun 29, 2017

Member

Might be worth asserting that the VM is also gone from virsh list --all

This comment has been minimized.

@mvollmer

mvollmer Jun 29, 2017

Member

Yes, fixed.

@mvollmer

mvollmer Jun 29, 2017

Member

Yes, fixed.

@martinpitt

Looks great now, thanks! So just needs a screenshot, and a commit message with a bit more verbosity :-)

}
export function deleteDialog(vm, dispatch) {
let values = {

This comment has been minimized.

@mareklibra

mareklibra Jun 30, 2017

Contributor

Seems like values are not reassigned later in the function, so const would be better here.
The const denotes that the reference can't be changed, but claims nothing about the object being referenced. I've seen this on other places within this PR.

Please feel free to ignore that since its unrelated to semantics.

@mareklibra

mareklibra Jun 30, 2017

Contributor

Seems like values are not reassigned later in the function, so const would be better here.
The const denotes that the reference can't be changed, but claims nothing about the object being referenced. I've seen this on other places within this PR.

Please feel free to ignore that since its unrelated to semantics.

@mvollmer

This comment has been minimized.

Show comment
Hide comment
@mvollmer

mvollmer Jun 30, 2017

Member

Screenshot:
virt-delete

Member

mvollmer commented Jun 30, 2017

Screenshot:
virt-delete

@garrett

This comment has been minimized.

Show comment
Hide comment
@garrett

garrett Jun 30, 2017

Contributor

LGTM! 😄

Contributor

garrett commented Jun 30, 2017

LGTM! 😄

@garrett garrett self-requested a review Jun 30, 2017

@garrett

Here's my official approval now.

(An emoji, a comment, and now this.)

@martinpitt

This comment has been minimized.

Show comment
Hide comment
@martinpitt

martinpitt Jul 1, 2017

Member

testDelete (check_machines.TestMachines) consistently fails. My first guess that this needs this fix from Stef -- if that doesn't help, it needs further investigation.

Member

martinpitt commented Jul 1, 2017

testDelete (check_machines.TestMachines) consistently fails. My first guess that this needs this fix from Stef -- if that doesn't help, it needs further investigation.

@mvollmer

This comment has been minimized.

Show comment
Hide comment
@mvollmer

mvollmer Jul 3, 2017

Member

testDelete (check_machines.TestMachines) consistently fails.

Seems to be okay now, no? (All green except ubuntu-stable, which seems to start all VMs in "paused".)

Member

mvollmer commented Jul 3, 2017

testDelete (check_machines.TestMachines) consistently fails.

Seems to be okay now, no? (All green except ubuntu-stable, which seems to start all VMs in "paused".)

@martinpitt

This comment has been minimized.

Show comment
Hide comment
@martinpitt

martinpitt Jul 3, 2017

Member

Right, that failure is due to some ominous test host issue which doesn't allow nested VMs (nothing to do with the tested OS). Last time I looked it still failed on the new delete test, so let's retry again.

Member

martinpitt commented Jul 3, 2017

Right, that failure is due to some ominous test host issue which doesn't allow nested VMs (nothing to do with the tested OS). Last time I looked it still failed on the new delete test, so let's retry again.

@martinpitt martinpitt merged commit b6c526f into cockpit-project:master Jul 3, 2017

16 checks passed

avocado/fedora-24 Tests passed
Details
container/kubernetes Tests passed
Details
selenium/chrome Tests passed
Details
selenium/firefox Tests passed
Details
semaphoreci The build passed on Semaphore.
Details
verify/centos-7 Tests passed
Details
verify/debian-stable Tests passed
Details
verify/debian-testing Tests passed
Details
verify/fedora-26 Tests passed
Details
verify/fedora-atomic Tests passed
Details
verify/fedora-i386 Tests passed
Details
verify/rhel-7 Tests passed
Details
verify/rhel-7-4 Tests passed
Details
verify/rhel-atomic Tests passed
Details
verify/ubuntu-1604 Tests passed
Details
verify/ubuntu-stable Tests passed
Details

@mareklibra mareklibra referenced this pull request Apr 27, 2018

Open

kvm management #9059

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment