Setting consul::version in hiera does not change the download_url about puppet-consul HOT 17 CLOSED

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

from puppet-consul.

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.

from puppet-consul.

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).

from puppet-consul.

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?

from puppet-consul.

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.

from puppet-consul.

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.

from puppet-consul.

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.

from puppet-consul.

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.

from puppet-consul.

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.

from puppet-consul.

I submitted PR 133 to address this. HTH

from puppet-consul.

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 :(

from puppet-consul.

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

from puppet-consul.

@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.

from puppet-consul.

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!

from puppet-consul.

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)

from puppet-consul.

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

from puppet-consul.

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

from puppet-consul.