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.

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

Make it possible to override base package URL location; switch default to GitHub #458

Merged
merged 11 commits into from Sep 26, 2017

Conversation

Projects
None yet
2 participants
@Wing924
Copy link
Contributor

commented Aug 30, 2017

Fixes #457, addresses RabbitMQ core team request to switch to Bintray/GitHub/Package Cloud from rabbitmq.com.

Wing924 added some commits Aug 30, 2017

@Wing924

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2017

@michaelklishin All tests are passed except lint test. But it seems not a part of my PR. Please check it.

@michaelklishin

This comment has been minimized.

Copy link
Member

commented Aug 30, 2017

This looks differently from what was originally suggested. @jjasghar how are such attribute initialization issues typically resolved in modern Chef? This approach seems pretty hacky to me.

@Wing924

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2017

@michaelklishin I tried lazy { ... }, but it don't work with embed string like "../#{node['rabbitmq'][...']}". If you have any better idea, please tell me.

@ghost

This comment has been minimized.

Copy link

commented Sep 16, 2017

There was already a back-and-forth on this issue back in 3.x. There was a change merged into 3.0.4 which moved URL string interpolation from from the attributes file into the recipe. However, the way it was done broke the ability to override package URL and use a local mirror.

Then later in 3.12.0 that behaviour was reverted and URL string interpolation was moved back into the attributes file which causes the behaviour that @Wing924 is trying to fix.

I believe the best approach is to do URL string interpolation in a recipe, while still allowing that to be overridden by an attribute if defined. It would look something like this:

attributes/default.rb:

default['rabbitmq']['version'] = '3.5.2'
default['rabbitmq']['deb_package'] = nil # override me if you have a local mirror

recipes/default.rb

default_deb_package = "https://www.rabbitmq.com/releases/rabbitmq-server/v#{node['rabbitmq']['version']}/"
deb_package = node['rabbitmq']['deb_package'] || default_deb_package
remote_file "#{Chef::Config[:file_cache_path]}/#{deb_package}" do
  source deb_package
@Wing924

This comment has been minimized.

Copy link
Contributor Author

commented Sep 22, 2017

@michaelklishin
I updated my PR as @yardinicwaller commented. Can you please check this PR?

@michaelklishin

This comment has been minimized.

Copy link
Member

commented Sep 25, 2017

@yardinicwaller thanks for the historical perspective and the suggestion. @Wing924 I'd need to investigate CI failures but let's change default URL to point to GitHub while we are at it. Thank you.

version = node['rabbitmq']['version']

default_deb_package_name = "rabbitmq-server_#{version}-1_all.deb"
default_deb_package_url = "https://www.rabbitmq.com/releases/rabbitmq-server/v#{version}/"

This comment has been minimized.

Copy link
@michaelklishin

michaelklishin Sep 25, 2017

Member

It's a good opportunity to switch this URL away fro mrabbitmq.com/releases, say, to GitHub releases. GitHub offers better availability, a CDN and there are not plans to drop it as an artifact distribution mechanism, unlike rabbitmq.com/releases.

@michaelklishin

This comment has been minimized.

Copy link
Member

commented Sep 25, 2017

CI failures should be addressed in master. @Wing924 please rebase/merge in master.

@michaelklishin

This comment has been minimized.

Copy link
Member

commented Sep 25, 2017

@Wing924 actually, I'm happy to change download URL to point to GitHub or Bintray separately, so let's merge in master to get CI failure fixes and see.

@jjasghar do you have any objections to the approach in this PR?

@michaelklishin

This comment has been minimized.

Copy link
Member

commented Sep 26, 2017

CI in master is green again.

Wing924 added some commits Sep 26, 2017

@michaelklishin

This comment has been minimized.

Copy link
Member

commented Sep 26, 2017

@Wing924 there is a CI failure that looks genuine:

* remote_file[/opt/kitchen/cache/rabbitmq-server_3.6.12-1_all.deb] action create_if_missing[2017-09-26T06:28:25+00:00] WARN: remote_file[/opt/kitchen/cache/rabbitmq-server_3.6.12-1_all.deb] cannot be downloaded from https://github.com/rabbitmq/rabbitmq-server/releases/download/v3.6.12/rabbitmq-server_3.6.12-1_all.deb: 404 "Not Found"

The correct URL is https://github.com/rabbitmq/rabbitmq-server/releases/download/rabbitmq_v3_6_12/rabbitmq-server_3.6.12-1_all.deb.

Bintray URLs should require no substitution: https://dl.bintray.com/rabbitmq/rabbitmq-server-deb/rabbitmq-server_3.6.12-1_all.deb.
We have plans to change Bintray repository for Debian packages to just deb but we can worry about that when it happens.

@michaelklishin

This comment has been minimized.

Copy link
Member

commented Sep 26, 2017

OK, looks like afcc286 addresses the above comment.

Wing924 added some commits Sep 26, 2017

@michaelklishin michaelklishin changed the title fix version is hard coded Make it possible to override base package URL location; switch default to GitHub Sep 26, 2017

@michaelklishin michaelklishin merged commit 30ef49f into rabbitmq:master Sep 26, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.