Fixes #21114 - Update error message #375

chris1984 · 2017-09-26T16:09:26Z

No description provided.

theforeman-bot · 2017-09-26T16:09:28Z

@chris1984, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

check #21114: Unfriendly error message when installing dhcp on interface without IP address: installer fails with pick(): must receive at least one non empty" in the logs is the correct one
move ticket #21114 from Foreman to the Installer project
or file a new ticket in the Installer project

If changing the ticket number used, remember to update the PR title and the commit message (using git commit --amend).

This message was auto-generated by Foreman's prprocessor

ekohl · 2017-09-26T16:24:26Z

manifests/proxydhcp.pp

@@ -3,7 +3,7 @@
  # puppet fact names are converted from ethX.X and ethX:X to ethX_X
  # so for alias and vlan interfaces we have to modify the name accordingly
  $interface_fact_name = regsubst($foreman_proxy::dhcp_interface, '[.:]', '_')
-  $ip   = pick($::foreman_proxy::dhcp_pxeserver, inline_template("<%= scope.lookupvar('::ipaddress_${interface_fact_name}') %>"))
+  $ip   = pick_default($::foreman_proxy::dhcp_pxeserver, inline_template("<%= scope.lookupvar('::ipaddress_${interface_fact_name}', '') %>"), '')


I don't think we should depend on puppetlabs-stdlib 4.19.0 already since it's very new, but with that it can be $ip = pick_default($::foreman_proxy::dhcp_pxeserver, fact("ipaddress_${interface_fact_name}")).

chris1984 · 2017-09-26T16:58:29Z

@ekohl thanks, fixed with the correction, can you look at it when you get another chance please?

iNecas · 2017-09-26T19:55:19Z

👍

ekohl · 2017-09-27T11:10:00Z

Could you add a testcase? Untested, but something adding the following to spec/classes/foreman_proxy__proxydhcp__spec.rb should do it:

context "on a non-existing interface" do
  let :pre_condition do
    "class { 'foreman_proxy':
      dhcp_interface => 'doesnotexist',
    }"
  end

  it { should raise_error(Puppet::Error, /Could not get the ip address from fact ipaddress_doesnotexist/) }
end

You should also increase the puppetlabs-stdlib minimum version to 4.19.0 in metadata.json since the function fact was introduced there.

chris1984 · 2017-09-27T19:51:49Z

@ekohl thanks for the help with the tests and the corrections, I have made the requested changes. Can you review when you get a chance, please?

ekohl

Looks good. Some minor style tweaks would be nice.

ekohl · 2017-09-27T19:58:55Z

spec/classes/foreman_proxy__proxydhcp__spec.rb

@@ -183,6 +183,16 @@
          }"
        end

+      context "on a non-existing interface" do


The indenting looks inconsistent.

I looked in Atom and it at spot 7 and the other contexts are as well

It looks like you pasted in the middle of another context. That's why the indenting looks incorrect.

ekohl · 2017-09-27T20:00:10Z

manifests/proxydhcp.pp

@@ -3,7 +3,7 @@
  # puppet fact names are converted from ethX.X and ethX:X to ethX_X
  # so for alias and vlan interfaces we have to modify the name accordingly
  $interface_fact_name = regsubst($foreman_proxy::dhcp_interface, '[.:]', '_')
-  $ip   = pick($::foreman_proxy::dhcp_pxeserver, inline_template("<%= scope.lookupvar('::ipaddress_${interface_fact_name}') %>"))
+  $ip = pick_default($::foreman_proxy::dhcp_pxeserver, fact("ipaddress_${interface_fact_name}"))
  if ! is_ip_address($ip) {
    fail("Could not get the ip address from fact ipaddress_${interface_fact_name}")
  }


Just below the fold here we have more inline_template use. Could you also replace those with fact to keep it consistent?

ekohl · 2017-09-27T20:02:17Z

manifests/proxydhcp.pp

@@ -3,7 +3,7 @@
  # puppet fact names are converted from ethX.X and ethX:X to ethX_X
  # so for alias and vlan interfaces we have to modify the name accordingly
  $interface_fact_name = regsubst($foreman_proxy::dhcp_interface, '[.:]', '_')
-  $ip   = pick($::foreman_proxy::dhcp_pxeserver, inline_template("<%= scope.lookupvar('::ipaddress_${interface_fact_name}') %>"))
+  $ip = pick_default($::foreman_proxy::dhcp_pxeserver, fact("ipaddress_${interface_fact_name}"))
  if ! is_ip_address($ip) {


While you're at it, mind replacing this with the non-deprecated version to remove a deprecation warning? I think unless $ip =~ Stdlib::Compat::Ipv4 { should work. Same for the other 3 occurrences.

chris1984 · 2017-09-28T15:51:55Z

@ekohl can I get another review please, also see the comment about the indenting.

ekohl · 2017-09-28T17:23:52Z

manifests/proxydhcp.pp

    fail("Could not get the ip address from fact ipaddress_${interface_fact_name}")
  }

-  $net  = inline_template("<%= scope.lookupvar('::network_${interface_fact_name}') %>")
-  if ! is_ip_address($net) {
+  $net  = fact("<%= scope.lookupvar('::network_${interface_fact_name}') %>")


This should be fact("network_${interface_fact_name}")

ekohl · 2017-09-28T17:24:03Z

manifests/proxydhcp.pp

    fail("Could not get the network address from fact network_${interface_fact_name}")
  }

-  $mask = inline_template("<%= scope.lookupvar('::netmask_${interface_fact_name}') %>")
-  if ! is_ip_address($mask) {
+  $mask = fact("<%= scope.lookupvar('::netmask_${interface_fact_name}') %>")


chris1984 · 2017-09-28T20:21:28Z

@ekohl the tests are failing since it cant find the ip, do i need to update the tests as well, I made the changes you suggested.

mmoll · 2017-09-29T19:01:48Z

merged, thanks @chris1984!

theforeman-bot added the Waiting on contributor label Sep 26, 2017

ekohl reviewed Sep 26, 2017

View reviewed changes

theforeman-bot removed the Waiting on contributor label Sep 26, 2017

ekohl requested changes Sep 27, 2017

View reviewed changes

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Sep 27, 2017

ekohl requested changes Sep 28, 2017

View reviewed changes

Fixes #21114 - Update error message

4a9ad28

ekohl approved these changes Sep 29, 2017

View reviewed changes

ekohl requested a review from mmoll September 29, 2017 18:34

mmoll approved these changes Sep 29, 2017

View reviewed changes

mmoll merged commit f291aa3 into theforeman:master Sep 29, 2017

chris1984 deleted the update_error branch September 29, 2017 20:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #21114 - Update error message #375

Fixes #21114 - Update error message #375

chris1984 commented Sep 26, 2017

theforeman-bot commented Sep 26, 2017

ekohl Sep 26, 2017

chris1984 commented Sep 26, 2017

iNecas commented Sep 26, 2017

ekohl commented Sep 27, 2017

chris1984 commented Sep 27, 2017

ekohl left a comment

ekohl Sep 27, 2017

chris1984 Sep 28, 2017

ekohl Sep 28, 2017

ekohl Sep 27, 2017

ekohl Sep 27, 2017

chris1984 commented Sep 28, 2017

ekohl Sep 28, 2017

ekohl Sep 28, 2017

chris1984 commented Sep 28, 2017

mmoll commented Sep 29, 2017

Fixes #21114 - Update error message #375

Fixes #21114 - Update error message #375

Conversation

chris1984 commented Sep 26, 2017

theforeman-bot commented Sep 26, 2017

Choose a reason for hiding this comment

chris1984 commented Sep 26, 2017

iNecas commented Sep 26, 2017

ekohl commented Sep 27, 2017

chris1984 commented Sep 27, 2017

ekohl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chris1984 commented Sep 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chris1984 commented Sep 28, 2017

mmoll commented Sep 29, 2017