Skip to content
This repository has been archived by the owner on Mar 6, 2024. It is now read-only.

Add method delete_all_nics to vm.py #788

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dwettstein
Copy link
Contributor

@dwettstein dwettstein commented Dec 15, 2021

This PR adds the method delete_all_nics to vm.py. The logic is basically the same as in the delete_nic method, except that instead of deleting just the NIC with given index, it deletes all.

NOTE: I have not added a dedicated test for this method, because of the same reason as above.

PS: I recommend to merge also PR #720, as it contains some bugfixes for delete_nic method.


This change is Reviewable

@vmwclabot
Copy link

@dwettstein, you must sign our contributor license agreement before your changes are merged. Click here to sign the agreement. If you are a VMware employee, read this for further instruction.

@vmwclabot
Copy link

@dwettstein, VMware has approved your signed contributor license agreement.

Copy link
Contributor

@rocknes rocknes left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @dwettstein and @rocknes)


pyvcloud/vcd/vm.py, line 757 at r1 (raw file):

                net_conn_section.remove(nc)
        else:
            raise InvalidStateException(

I think we don't need to raise an exception if no nics are found.

Copy link
Contributor Author

@dwettstein dwettstein left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @rocknes)


pyvcloud/vcd/vm.py, line 757 at r1 (raw file):

Previously, rocknes wrote…

I think we don't need to raise an exception if no nics are found.

Thanks @rocknes, you're right, I have removed the else part.

@dwettstein dwettstein requested a review from rocknes February 14, 2022 22:09
@dwettstein
Copy link
Contributor Author

Hi @rocknes

I have implemented your requested changes. Is there something left to do for this PR?

Thanks.

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants