Setting consul::version in hiera does not change the download_url

[centromere]

I attempted to set consul::version to 0.5.2 in hiera, but the zip file downloaded is for 0.5.0. I added the following to install.pp:

notify{"Download url: ${$consul::download_url}": }
notify{"Version: ${$consul::version}": }

And this is the output:

Notice: Download url: https://dl.bintray.com/mitchellh/consul/0.5.0_linux_amd64.zip
Notice: Version: 0.5.2

[solarkennedy]

Gosh, that is pretty surprising, but not the first time I've heard of this.
We have a specific test for this?
https://github.com/solarkennedy/puppet-consul/blob/master/spec/classes/init_spec.rb#L78-L83

[EvanKrall]

I think it's that staging::file won't re-download something if the file (zipfile, in our case) already exists; nor will staging::extract extract something if the target already exists.

[centromere]

I am not certain it is with staging::file. I suspect it might have something to do with the variables getting substituted in an unexpected way (as demonstrated by the test I did).

[solarkennedy]

Yea, it can't be the staging module, this is happening at catalog time.

@centromere can you give us more inputs to work with? Can you paste in more for your hiera and manifests to give us a better picture of how this module is invoked?

[centromere]

Puppet 3.7.4

site.pp:

node /([a-z]+)\d+$/ {
  $node_role = $1
  hiera_include('classes')
}

hiera.yaml:

:hierarchy:
  - "roles/%{node_role}"
  - common

hiera/common.yaml:

---
classes:
  - myapp_discovery

consul::version: "0.5.2"

inhouse/myapp_discovery/manifests/init.pp:

class myapp_discovery {
  include consul
}

Setting consul::download_url instead of consul::version results in the desired behavior.

[EvanKrall]

If instead of setting consul::version in hiera, you pass version => '0.5.2' when you include consul, does the same behavior happen?

You're right, this isn't staging. It might have to do with the ordering of when $download_url's default gets constructed versus when $version gets set.

[solarkennedy]

I can confirm on a spec test that inserting the version works. It does have something to do with the version parameter being inherited early before the hiera lookup.

I'm trying to write an failing integration test to reproduce.

[potto007]

I believe this is a side-effect of this 3+ year old issue, which has burned me a number of times in Puppet.

https://projects.puppetlabs.com/issues/9848

Anytime I assign a variable based on other variables inside the parameter definition of the class, it winds up yielding unpredictable results.

[solarkennedy]

Ah! That explains it. I swear I've seen this work before, but I used another internal module today where I tried to do the same thing and it didn't work.

Well, I would accept a PR for something like this:

$version = 0.5.0
$download_url = undef,
) {
  $real_download_url = pick($download_url, 'http://..$version')

That way we don't bother composing the default url unless it wasn't provided, and we parse $version only when we are certain when it is set.

[potto007]

I submitted PR 133 to address this. HTH

[solarkennedy]

I've merged it in. Thank you @potto007 ! (especially considering you didn't open the original issue!)

I'm sad I couldn't come up with a test to verify this. I can't guarantee this particular functionality won't be broken in the future without a unit test to watch for it :(

[solarkennedy]

@centromere can you pull master and confirm that this closes your issue?

[potto007]

@solarkennedy I'll see if I can come up with a few tests to assert this works, and see if I can make at least one that breaks the old code.

Glad to help, I needed it anyway in my infrastructure. :) I'm actually adding something for managing a Docker container based Consul via Upstart ATM - I'll contribute that as a branch when it is clean and see if you want it.

[solarkennedy]

Cool! @potto007 We do have a test on inserting the download_url already, but it doesn't work for this bug because this require more of a kind of "integration" between hiera and junk. A real test of this will require the hiera helper and using similar hiera that @centromere provided. Good luck!

Regarding docker/consul/upstart, that kinda sounds a bit out of scope maybe could be its own module, or an example? But yea I would be happy to see it!

[potto007]

Yeah, I've done some tests with Hiera involved, I think it should be manageable.

WRT consul-docker: It does seem out of scope, I agree - but the way I am using it, your module actually is quite helpful. I am using host volume mapping against /etc/consul - and Puppet is being used to assign service definitions. Obviously, consul::install doesn't much matter for that case, and service management is not needed in the classical sense... but I still want something to prevent manual intervention, or one-off orchestration kick-off.

The whole reason I have to use Docker in the particular environment I'm now deploying to: it has no private IPs - and I have no control over how our datacenter implements things in that location. Consul doesn't like that type of setup, from what I've learned through my efforts and through reading of GitHub Issues. Consul-Docker gets around this obviously because the bridge network is the "private network" and a simple advertise_addr param with the host's public IP fixes any problems. (hashicorp/consul#725 - for the curious)

[hopperd]

I believe this issue is now fixed are we ok to close this?

[hopperd]

Closing as I believe this is fixed now, please re-open if that isn't the case.