Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Upgrade and Downgrade with url #118

Closed
wants to merge 1 commit into from
+33 −20
Closed

Upgrade and Downgrade with url #118

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 25 additions & 12 deletions manifests/install.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,37 +15,50 @@

if $consul::install_method == 'url' {

$version = $consul::version
$consul_archive_target = "consul_${version}.zip"
$consul_web_ui_archive_target = "consul_web_ui_${version}.zip"

if $::operatingsystem != 'darwin' {
ensure_packages(['unzip'])
}
staging::file { 'consul.zip':
staging::file { $consul_archive_target:
source => $consul::download_url
} ->
staging::extract { 'consul.zip':
target => $consul::bin_dir,
creates => "${consul::bin_dir}/consul",
file { "${consul::bin_dir}/consul_${version}/":
ensure => 'directory',
owner => 'root',
group => 0,
mode => '0755',
} ->
staging::extract { $consul_archive_target:
target => "${consul::bin_dir}/consul_${version}/",
creates => "${consul::bin_dir}/consul_${version}/consul",
} ->
file { "${consul::bin_dir}/consul":
owner => 'root',
group => 0, # 0 instead of root because OS X uses "wheel".
mode => '0555',
owner => 'root',
group => 0, # 0 instead of root because OS X uses "wheel".
mode => '0555',
target => "${consul::bin_dir}/consul_${version}/consul",
notify => Class['consul::run_service'],
}


if ($consul::ui_dir and $consul::data_dir) {
file { "${consul::data_dir}/${consul::version}_web_ui":
file { "${consul::data_dir}/${version}_web_ui":
ensure => 'directory',
owner => 'root',
group => 0, # 0 instead of root because OS X uses "wheel".
mode => '0755',
} ->
staging::deploy { 'consul_web_ui.zip':
staging::deploy { $consul_web_ui_archive_target:
source => $consul::ui_download_url,
target => "${consul::data_dir}/${consul::version}_web_ui",
creates => "${consul::data_dir}/${consul::version}_web_ui/dist",
target => "${consul::data_dir}/${version}_web_ui",
creates => "${consul::data_dir}/${version}_web_ui/dist",
}
file { $consul::ui_dir:
ensure => 'symlink',
target => "${consul::data_dir}/${consul::version}_web_ui/dist",
target => "${consul::data_dir}/${version}_web_ui/dist",
}
}

Expand Down
2 changes: 1 addition & 1 deletion manifests/params.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
$package_ensure = 'latest'
$ui_package_name = 'consul_ui'
$ui_package_ensure = 'latest'
$version = '0.5.0'
$version = hiera('consul::version', '0.5.0') # ensure defaults resolve from hiera as well
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not advised to call Hiera from a Module ...

Best practices are to use the Role / Profile pattern to compose nodes definition and make Hiera calls from these classes which are business specific anyway .

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In our puppet setup we have various roles that use the Consul module. We don't want to write the Consul version in each of the roles (or their hiera counterpart), but want to set them once per environment (staging, production etc). The old code forces the user of the Consul module to explicitly pass the version for Consul to the module in order to create the correct download url, whereas the actual version in the init.pp would be set from hiera. This seems highly counter-intuitive to me. If I set consul::version in Hiera, I would expect that version to be downloaded and used. We don't have the concept of Profiles.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also going to veto an explicit heira lookup here.

I don't understand why $consul::version in hiera would not be picked up using the native hiera bindings?

It may be counter-intuitive to you, but this is the documented way that variable precedence takes place:
https://docs.puppetlabs.com/hiera/1/puppet.html#automatic-parameter-lookup

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$consul::version works fine
but in init.pp we have defaults:

$download_url = "https://dl.bintray.com/mitchellh/consul/${version}_${os}_${arch}.zip",
$ui_download_url = "https://dl.bintray.com/mitchellh/consul/${version}_web_ui.zip",

These defaults apparently do not get populated by hiera. Eg if you have in hiera:
consul::version: 0.5.1 and in params.pp $version = '0.5.0' this means your url will become https://dl.bintray.com/mitchellh/consul/0.5.0_web_ui.zip instead of 0.5.1 (which is used in other places because init.pp:version is correctly resolved from hiera)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something must be weird with your hiera setup then.

We use hiera exactly like you mention:

Eg in hiera

consul::version: 0.5.1

and in params.pp

$version = '0.5.0'

and the download url becomes:

https://dl.bintray.com/mitchellh/consul/0.5.1_web_ui.zip

Edit:
Oh, do you mean those urls as params in init.pp?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, do you mean those urls as params in init.pp?

Not sure what you mean, but end-result is that if I only set consul::version in hiera, my download comes from the version-number as set in params. I'll test it again!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we still blocked on this? Can we keep this the way it was (without hiera) and merge it?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thijsterlouw I think the problem you are experiencing with hiera is fixed here #129


case $::architecture {
'x86_64', 'amd64': { $arch = 'amd64' }
Expand Down
14 changes: 7 additions & 7 deletions spec/classes/init_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
}}
it { expect { should compile }.to raise_error(/is not a boolean/) }
end

context 'When passing a non-bool as manage_service' do
let(:params) {{
:manage_service => 'hello'
Expand Down Expand Up @@ -72,21 +72,21 @@
end

context "When installing via URL by default" do
it { should contain_staging__file('consul.zip').with(:source => 'https://dl.bintray.com/mitchellh/consul/0.5.0_linux_amd64.zip') }
it { should contain_staging__file('consul_0.5.0.zip').with(:source => 'https://dl.bintray.com/mitchellh/consul/0.5.0_linux_amd64.zip') }
end

context "When installing via URL by with a special version" do
let(:params) {{
:version => '42',
}}
it { should contain_staging__file('consul.zip').with(:source => 'https://dl.bintray.com/mitchellh/consul/42_linux_amd64.zip') }
it { should contain_staging__file('consul_42.zip').with(:source => 'https://dl.bintray.com/mitchellh/consul/42_linux_amd64.zip') }
end

context "When installing via URL by with a custom url" do
let(:params) {{
:download_url => 'http://myurl',
}}
it { should contain_staging__file('consul.zip').with(:source => 'http://myurl') }
it { should contain_staging__file('consul_0.5.0.zip').with(:source => 'http://myurl') }
end


Expand Down Expand Up @@ -117,7 +117,7 @@
'ui_dir' => '/dir1/dir2',
},
}}
it { should contain_staging__file('consul_web_ui.zip').with(:source => 'https://dl.bintray.com/mitchellh/consul/0.5.0_web_ui.zip') }
it { should contain_staging__file('consul_web_ui_0.5.0.zip').with(:source => 'https://dl.bintray.com/mitchellh/consul/0.5.0_web_ui.zip') }
end

context "When installing UI via URL by with a special version" do
Expand All @@ -128,7 +128,7 @@
'ui_dir' => '/dir1/dir2',
},
}}
it { should contain_staging__file('consul_web_ui.zip').with(:source => 'https://dl.bintray.com/mitchellh/consul/42_web_ui.zip') }
it { should contain_staging__file('consul_web_ui_42.zip').with(:source => 'https://dl.bintray.com/mitchellh/consul/42_web_ui.zip') }
end

context "When installing UI via URL by with a custom url" do
Expand All @@ -139,7 +139,7 @@
'ui_dir' => '/dir1/dir2',
},
}}
it { should contain_staging__deploy('consul_web_ui.zip').with(:source => 'http://myurl') }
it { should contain_staging__deploy('consul_web_ui_0.5.0.zip').with(:source => 'http://myurl') }
end

context "By default, a user and group should be installed" do
Expand Down