enable dhcp as optional parameter (typelist) #385

MalloZup · 2018-08-30T10:19:43Z

This pr contain:

dhcp backport from [WIP] More configuration options for networks #288.
documentation.

Additionaly to the #288 we have:

real testing for real the dhcp parameter via xml property of network
- we are testing that the xml networkDefinition have dhcp set or not (depending on dhcp nabl/disable)

via xml def

Bischoff · 2018-09-01T09:19:35Z

@MalloZup

The doc is not okay: addresses can be used for other things than DHCP, for example for setting an interface to the private network's bridge:

resource "libvirt_network" "private_network" {
  name = "ebi3-private"
  mode = "none"
  addresses = [ "192.168.5.0/24" ]
  dhcp { enabled = "false" }
}

produces:

<network connections='1'>
  <name>ebi3-private</name>
  <uuid>43fec82e-4129-45ac-b262-808cd71b4c44</uuid>
  <bridge name='virbr1' stp='on' delay='0'/>
  <mac address='52:54:00:65:a1:2e'/>
  <ip family='ipv4' address='192.168.5.1' prefix='24'>
  </ip>
</network>

so you get 192.168.5.1 on virbr1, which is nice.

The example above produces a terraform crash.

panic: runtime error: invalid memory address or nil pointer dereference
[DEBUG] plugin.terraform-provider-libvirt: [signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0xc5419c]
[DEBUG] plugin.terraform-provider-libvirt: 
[DEBUG] plugin.terraform-provider-libvirt: goroutine 227 [running]:
[DEBUG] plugin.terraform-provider-libvirt: libvirt.getDomainInterfacesFromNetworks(0x0, 0x0, 0x0, 0x0, 0x101dc98, 0x3, 0x0, 0xc420478af0, 0xe, 0x0, ...)
[DEBUG] plugin.terraform-provider-libvirt: libvirt/domain.go:232 +0x6cc
[DEBUG] plugin.terraform-provider-libvirt: libvirt.domainGetIfacesInfo(0x34c16d0, 0x0, 0x0, 0x0, 0x0, 0x101dc98, 0x3, 0x0, 0xc420478af0, 0xe, ...)
[DEBUG] plugin.terraform-provider-libvirt: libvirt/domain.go:170 +0x1e0
[DEBUG] plugin.terraform-provider-libvirt: libvirt.domainIfaceHasAddress(0x34c16d0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0xc42046c990, ...)

Bischoff · 2018-09-01T12:44:07Z

@MalloZup just pick the fixes to the crash mentioned above from my own PR, it boils down to adding two tests:

https://github.com/dmacvicar/terraform-provider-libvirt/pull/321/files#diff-f6f8db75203b0db83110f024f21cc4eb#232 if ips.DHCP != nil ...
https://github.com/dmacvicar/terraform-provider-libvirt/pull/321/files#diff-042e2374df248e4277a3506b065eb28a#243 if enableDHCP { ...

I think it will not require much adaptation.

You may also see in my PR for better documentation.

MalloZup · 2018-09-01T16:02:33Z

libvirt/resource_libvirt_network_test.go

@@ -130,3 +133,96 @@ func TestAccLibvirtNetwork_Import(t *testing.T) {
 		},
 	})
 }
+
+func TestAccLibvirtNetwork_EnableDhcpAndDisableItAfter(t *testing.T) {


rename this

To whom are you speaking to? 😸

i just reviewed my code myself 🚀 🤖 was a sidenote 🤣

Please don't get angry with your reviewer 😋 . Enjoy your week-end too 🎆 .

dmacvicar · 2018-09-02T09:21:49Z

libvirt/resource_libvirt_network.go

@@ -131,28 +147,28 @@ func resourceLibvirtNetworkUpdate(d *schema.ResourceData, meta interface{}) erro
 	}
 	network, err := virConn.LookupNetworkByUUIDString(d.Id())
 	if err != nil {
-		return err
+		return fmt.Errorf("Error by retrieving network ID during update: %s", err)


Here you are not retrieving a network ID, but looking a network by its ID. I would suggest:

fmt.Errorf("Can't retrieve network with UUID '%s' during update: %s", uuid, err)

dmacvicar · 2018-09-02T16:18:24Z

libvirt/resource_libvirt_network_test.go

@@ -81,6 +42,48 @@ func networkExists(n string, network *libvirt.Network) resource.TestCheckFunc {
 	}
 }

+func testAccCheckLibvirtNetworkDhcpStatus(name string, network *libvirt.Network, DhcpStatus string) resource.TestCheckFunc {


Why is DhcpStatus camel-cased? (and wondering... why go format or vet do not complain?)

Speaking of this... In the comments, "dhcp" should be "DHCP" 😃

MalloZup · 2018-09-03T08:41:34Z

@Bischoff in reply to your 2nd comment:

https://github.com/dmacvicar/terraform-provider-libvirt/pull/321/files#diff-042e2374df248e4277a3506b065eb28a#243 if enableDHCP { ...

This is already here:

https://github.com/dmacvicar/terraform-provider-libvirt/pull/385/files#diff-042e2374df248e4277a3506b065eb28aR458

if the dhcp is not enabled we set it to nil which is equivalent to your suggestion.

For the domain.go yop i added the check 👍 Documentation is also improved/

@dmacvicar it is interesting the golint/go vet didn"t catch the "not formatted" function variable, i think it can be maybe an issue upstream.

Feel free to review and test it. TIA

Bischoff · 2018-09-03T08:52:00Z

@MalloZup ok for the second comment already applied.

For the first comment, I applied it locally, and terraform does not crash anymore.

Doc looks perfect now, thank you.

MalloZup added 3 commits August 30, 2018 20:54

enable dhcp paramater as optional typelist

6c4b831

add real test for dhcp ( testing no dhcp enabled or enabled)

482dc22

via xml def

add real tests for dhcp netifaces

a91d424

MalloZup force-pushed the dhcp-backport branch from e65f5ac to 6bde519 Compare August 30, 2018 18:55

MalloZup changed the title ~~WIP: dhcp backport~~ enable dhcp as optional parameter (typelist) Aug 30, 2018

MalloZup requested review from dmacvicar and inercia August 30, 2018 18:57

MalloZup changed the title ~~enable dhcp as optional parameter (typelist)~~ WIP: enable dhcp as optional parameter (typelist) Aug 30, 2018

MalloZup mentioned this pull request Aug 30, 2018

Decouple definition of subnet addresses from definition of DHCP range #321

Closed

MalloZup force-pushed the dhcp-backport branch 2 times, most recently from 4c7ccfa to 68e90bb Compare August 31, 2018 13:47

add documentation for dhcp

bf1c495

MalloZup force-pushed the dhcp-backport branch from 68e90bb to bf1c495 Compare August 31, 2018 13:49

MalloZup changed the title ~~WIP: enable dhcp as optional parameter (typelist)~~ enable dhcp as optional parameter (typelist) Aug 31, 2018

MalloZup commented Sep 1, 2018

View reviewed changes

dmacvicar requested changes Sep 2, 2018

View reviewed changes

improve naming of function/vars network

10fc80a

MalloZup force-pushed the dhcp-backport branch from 5934e04 to 10fc80a Compare September 3, 2018 08:19

check in domain if dhcp var is nil

1ad52dd

improve documentation dhcp

ced540d

MalloZup force-pushed the dhcp-backport branch from 5c8623f to ced540d Compare September 3, 2018 08:57

dmacvicar approved these changes Sep 3, 2018

View reviewed changes

inercia approved these changes Sep 3, 2018

View reviewed changes

inercia merged commit c46f0f8 into dmacvicar:master Sep 3, 2018

MalloZup deleted the dhcp-backport branch September 10, 2018 09:00

MalloZup mentioned this pull request Sep 14, 2018

changelog summary for new released version #412

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enable dhcp as optional parameter (typelist) #385

enable dhcp as optional parameter (typelist) #385

MalloZup commented Aug 30, 2018 •

edited

Loading

Bischoff commented Sep 1, 2018 •

edited

Loading

Bischoff commented Sep 1, 2018 •

edited

Loading

MalloZup Sep 1, 2018

Bischoff Sep 1, 2018

MalloZup Sep 1, 2018

Bischoff Sep 1, 2018

dmacvicar Sep 2, 2018

dmacvicar Sep 2, 2018

Bischoff Sep 3, 2018

MalloZup commented Sep 3, 2018

Bischoff commented Sep 3, 2018 •

edited

Loading

enable dhcp as optional parameter (typelist) #385

enable dhcp as optional parameter (typelist) #385

Conversation

MalloZup commented Aug 30, 2018 • edited Loading

Bischoff commented Sep 1, 2018 • edited Loading

Bischoff commented Sep 1, 2018 • edited Loading

MalloZup Sep 1, 2018

Choose a reason for hiding this comment

Bischoff Sep 1, 2018

Choose a reason for hiding this comment

MalloZup Sep 1, 2018

Choose a reason for hiding this comment

Bischoff Sep 1, 2018

Choose a reason for hiding this comment

dmacvicar Sep 2, 2018

Choose a reason for hiding this comment

dmacvicar Sep 2, 2018

Choose a reason for hiding this comment

Bischoff Sep 3, 2018

Choose a reason for hiding this comment

MalloZup commented Sep 3, 2018

Bischoff commented Sep 3, 2018 • edited Loading

MalloZup commented Aug 30, 2018 •

edited

Loading

Bischoff commented Sep 1, 2018 •

edited

Loading

Bischoff commented Sep 1, 2018 •

edited

Loading

Bischoff commented Sep 3, 2018 •

edited

Loading