-
Notifications
You must be signed in to change notification settings - Fork 289
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
(GH-688) Default sensu-plugin gem to use sensu_gem provider #694
(GH-688) Default sensu-plugin gem to use sensu_gem provider #694
Conversation
resolves #688 |
Note, this PR also changes |
@@ -56,15 +55,26 @@ | |||
|
|||
it { should contain_package('sensu-plugin').with( | |||
:ensure => 'installed', | |||
:provider => 'gem' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be changed to sensu_gem instead of removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same reasoning as line 43
@@ -40,7 +40,6 @@ | |||
|
|||
it { should contain_package('sensu-plugin').with( | |||
:ensure => 'installed', | |||
:provider => 'gem' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be changed to sensu_gem instead of removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about that, but I think specifying the provider here is superfluous since the context of the test, "setting version with / without platform suffix" doesn't seem to relate behaviorally to the provider. I would have left it the provider wasn't fully covered in the embedded_ruby context. Happy to add them back if you prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree.
spec/classes/sensu_package_spec.rb
Outdated
it { should contain_package('sensu-plugin').with(:provider => 'sensu_gem') } | ||
end | ||
|
||
context 'with use_embedded_ruby => true' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
context should read false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
The docs should also mention that we use the embedded ruby by default and do not recommend using the system ruby. @cwjohnston @agoddard should we even allow system ruby? |
Without this patch the default behavior is to manage sensu-plugin using the system `gem` command via the gem provider. This is a problem because this version combination is unknown and untested. The embedded ruby should be used for supportability and predictable behavior. This patch changes the default behavior to use the embedded ruby version at `/opt/sensu/embedded/bin/gem` via the `sensu_gem` provider shipped in this module. closes sensu#688
04022e4
to
44aae14
Compare
Docs updated in 44aae14 |
Thanks @jeffmccune ! Released in v2.7.0 |
You're welcome! Glad to help out, thanks for the quick review! |
Without this patch the behavior of sensu-plugin is managed by the default
package provider for the system, e.g. gem. This patch changes the default
behavior to use the embedded ruby version at
/opt/sensu/embedded/bin/gem
viathe built in provider.