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

addition of attributes for downloading deb, rpm and esl-erlang-compat #220

Merged
merged 2 commits into from Apr 6, 2015

Conversation

Projects
None yet
7 participants
@dannietjoh
Contributor

dannietjoh commented Feb 11, 2015

added attributes that allow configuration of the download url and package name for rabbitmq deb and rpm and esl-erlang-compat rpm, for example for those that do not have the luxury to connect production environments to the outside world.

@jjasghar

This comment has been minimized.

Show comment
Hide comment
@jjasghar

jjasghar Feb 11, 2015

Collaborator

Please add some specs for these changes.

Collaborator

jjasghar commented Feb 11, 2015

Please add some specs for these changes.

@dannietjoh

This comment has been minimized.

Show comment
Hide comment
@dannietjoh

dannietjoh Feb 11, 2015

Contributor

Hi,

I'll be glad to but I'm not quite sure how to add more to the spec tests than is provided, both rabbitmq deb and rpm files already have checks for availability in the cache path and checks for installation.
The esl-erlang-compat package depends on attributes from the erlang cookbook making it hard to build a spec for.

Contributor

dannietjoh commented Feb 11, 2015

Hi,

I'll be glad to but I'm not quite sure how to add more to the spec tests than is provided, both rabbitmq deb and rpm files already have checks for availability in the cache path and checks for installation.
The esl-erlang-compat package depends on attributes from the erlang cookbook making it hard to build a spec for.

@jujugrrr

This comment has been minimized.

Show comment
Hide comment
@jujugrrr

jujugrrr Mar 27, 2015

Please add some specs for these changes.

It seems there are already specs to test we are getting the appropriate files :
https://github.com/jjasghar/rabbitmq/blob/master/spec/default_spec.rb#L89-L95
https://github.com/jjasghar/rabbitmq/blob/master/spec/default_spec.rb#L119-L127

If this PR passes tests, it means the new attributes are not breaking the expected/previous behaviour, but allow to change the source if you need to, which is good.

We could add some context in the specs changing the attributes and testing it's not using parameters for remote_files, but I'm not sure of the value to do so.

In addition in the specs we already use the version https://github.com/jjasghar/rabbitmq/blob/master/spec/default_spec.rb#L79. , so those new attribute are tested.

We really need this PR, because lately https://www.rabbitmq.com/releases/rabbitmq-server has been really unreliable for us.

ps : thanks for the Pr @dannietjoh

jujugrrr commented Mar 27, 2015

Please add some specs for these changes.

It seems there are already specs to test we are getting the appropriate files :
https://github.com/jjasghar/rabbitmq/blob/master/spec/default_spec.rb#L89-L95
https://github.com/jjasghar/rabbitmq/blob/master/spec/default_spec.rb#L119-L127

If this PR passes tests, it means the new attributes are not breaking the expected/previous behaviour, but allow to change the source if you need to, which is good.

We could add some context in the specs changing the attributes and testing it's not using parameters for remote_files, but I'm not sure of the value to do so.

In addition in the specs we already use the version https://github.com/jjasghar/rabbitmq/blob/master/spec/default_spec.rb#L79. , so those new attribute are tested.

We really need this PR, because lately https://www.rabbitmq.com/releases/rabbitmq-server has been really unreliable for us.

ps : thanks for the Pr @dannietjoh

@kgeorgak

This comment has been minimized.

Show comment
Hide comment
@kgeorgak

kgeorgak Mar 27, 2015

I agree with the comments made by @jujugrrr.

The existing tests should be enough as they test the creation of the remote file. If the attributes don't have the correct value the test will flag it.

kgeorgak commented Mar 27, 2015

I agree with the comments made by @jujugrrr.

The existing tests should be enough as they test the creation of the remote file. If the attributes don't have the correct value the test will flag it.

@cmluciano

This comment has been minimized.

Show comment
Hide comment
@cmluciano

cmluciano Mar 27, 2015

Collaborator

I cherry-picked this and am testing now

Collaborator

cmluciano commented Mar 27, 2015

I cherry-picked this and am testing now

@nakedpony

This comment has been minimized.

Show comment
Hide comment
@nakedpony

nakedpony Mar 28, 2015

It's a really nice PR especially now when rabbitmq artifacts are non accessible on official site. Please read https://groups.google.com/forum/#!topic/rabbitmq-users/Ov0k_Xp0C3Y

nakedpony commented Mar 28, 2015

It's a really nice PR especially now when rabbitmq artifacts are non accessible on official site. Please read https://groups.google.com/forum/#!topic/rabbitmq-users/Ov0k_Xp0C3Y

@Igorshp

This comment has been minimized.

Show comment
Hide comment
@Igorshp

Igorshp Mar 30, 2015

👍 looking forward to seeing this merged.
the RabbitMQ hosting maintenance, is causing problems for our chef runs.

Igorshp commented Mar 30, 2015

👍 looking forward to seeing this merged.
the RabbitMQ hosting maintenance, is causing problems for our chef runs.

@cmluciano

This comment has been minimized.

Show comment
Hide comment
@cmluciano

cmluciano Mar 30, 2015

Collaborator

working on getting this released this week. Thank you for your patience.

Collaborator

cmluciano commented Mar 30, 2015

working on getting this released this week. Thank you for your patience.

@jjasghar jjasghar removed the needs tests label Apr 6, 2015

jjasghar pushed a commit that referenced this pull request Apr 6, 2015

JJ Asghar
Merge pull request #220 from dannietjoh/master
addition of attributes for downloading deb, rpm and esl-erlang-compat

@jjasghar jjasghar merged commit 5aa1295 into rabbitmq:master Apr 6, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@cmluciano cmluciano referenced this pull request Apr 7, 2015

Closed

erlang broken #199

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment