Skip to content
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

bugfix for vapp.RemoveNetwork() and vapp.RemoveNetworkAsync() when removing multiple network sequentially #299

Merged
merged 1 commit into from
Apr 10, 2020

Conversation

Didainius
Copy link
Collaborator

@Didainius Didainius commented Apr 9, 2020

This PR attempts to fix vapp.RemoveNetworkAsync() and vapp.RemoveNetwork() functions by using http DELETE call on endpoint https://xxx/api/network/ef005324-a62b-4041-ae8c-c7bec64fb348 instead of re-applying whole vApp network configuration with a particular network excluded.
It has caused situation where re-applying whole vApp network config gave error because that config was invalid.

One particular place which got impacted by that is Terraform provider with many sequential removals - https://github.com/terraform-providers/terraform-provider-vcd/issues/473

Copy link
Contributor

@vbauzys vbauzys 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
Collaborator

@lvirbalas lvirbalas left a comment

Choose a reason for hiding this comment

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

One ask to ammend changelog and LGTM!

@@ -11,6 +11,8 @@ NOTES:
BUGS FIXED:
* Fix issue in Queries with vCD 10 version, which do not return network pool or provider VDC[#293](https://github.com/vmware/go-vcloud-director/pull/293)
* Session timeout for media, catalog item upload [#294](https://github.com/vmware/go-vcloud-director/pull/294)
* Fix `vapp.RemoveNetwork`, `vapp.RemoveNetworkAsync` to use `DELETE` API call instead of update
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about a comment that this is actual only for 10.1?

Copy link
Collaborator Author

@Didainius Didainius Apr 9, 2020

Choose a reason for hiding this comment

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

It causes problems not only in 10.1. At least 10.0 and 9.5 also has problems with Terraform's binary test vcd.TestAccVcdVAppVmMultiNIC.tf reendering error similar to https://github.com/terraform-providers/terraform-provider-vcd/issues/473 . I have also tested the example in the issue and it failed on vCD 9.5 with 2.7.0.

Copy link
Contributor

@dataclouder dataclouder left a comment

Choose a reason for hiding this comment

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

LGTM

@Didainius Didainius merged commit 901273c into vmware:master Apr 10, 2020
@Didainius Didainius deleted the fix_vapp_removeNetwork branch April 10, 2020 05:59
Didainius added a commit to vmware/terraform-provider-vcd that referenced this pull request Apr 10, 2020
This PR bumps govcd dependency to support 10.1 and also:
* Adds a note about NSX-T not being supported yet
* Adds a support deprecation warning during apply phase for vCD <=9.1
* Adds a link to changelog in GitHub in main documentation page (https://www.terraform.io/docs/providers/vcd/index.html)
* Removes 9.1 from the list of supported versions and adds 10.1 in https://www.terraform.io/docs/providers/vcd/index.html
* Uses "Undeploy()" instead of PowerOff() for VMs during update phase
* Closes #473 by pulling in vmware/go-vcloud-director#299 .
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