search

dmacvicar / terraform-provider-libvirt

Terraform provider to provision infrastructure with Linux's KVM using libvirt
Apache License 2.0
1.54k stars 457 forks source link

Rework NetworkUpdate workaround #950

Closed cfergeau closed 1 year ago

cfergeau commented 2 years ago

The libvirt version check which is currently to decide whether to swap libvirt.NetworkUpdate arguments is not enough. The fix was backported to older libvirt in RHEL/CentOS (for example in libvirt-6.0.0-37.1.module+el8.5.0+13858+39fdc467 ), and the current version checking code would be swapping the arguments when it's no longer needed.

This commit uses the same code as libvirt client libraries instead, it checks if the libvirt connection supports the new NetworkUpdate call or not. I've tested this code with both libvirt-6.0.0-37.1 and libvirt-6.0.0-37 and in each case, the correct call was done to libvirt.NetworkUpdate

Please make sure you read the contributor documentation before opening a Pull Request.

cfergeau commented 2 years ago

I've updated this to make use of the new NetworkUpdateCompat which takes care of this at the digitalocean/go-libvirt level.

ElCoyote27 commented 2 years ago

Thank you @cfergeau ! I hope it gets merged soonish

cfergeau commented 1 year ago

@dmacvicar is this missing anything to get reviewed?

dmacvicar commented 1 year ago

Thanks. Can you cherry-pick 0b2f7083058b680210af2abf06944f0513c083f3 and 7a733d242f45a91c50d25c3f97fbf11bcbde5b31 from dmacvicar-cfergeau-network-update to get the pipeline passing?

cfergeau commented 1 year ago

Thanks. Can you cherry-pick 0b2f708 and 7a733d2 from dmacvicar-cfergeau-network-update to get the pipeline passing?

Done! I squashed the commit fixing linting in Remove NetworkUpdate workaround, if you prefer to keep it separate to keep authorship info, let me know!

NB: since 'Allow edits by maintainers' is checked, you could add a cfergeau remote set to git@github.com:cfergeau/terraform-provider-libvirt.git and push directly to my network-update branch instead of creating an intermediate origin/dmacvicar-cfergeau-network-update one.