Os x support #1

Merged
merged 3 commits into from Apr 3, 2012

Conversation

Projects
None yet
3 participants
Owner

capoferro commented Apr 3, 2012

This doesn't feel very clean, but it works. Would appreciate feedback.

@jhowarth jhowarth commented on an outdated diff Apr 3, 2012

@@ -9,6 +9,8 @@
supports os
end
+supports 'mac_os_x'
@jhowarth

jhowarth Apr 3, 2012

Member

This should probably be added to the %w a couple lines above this.

@jhowarth jhowarth commented on the diff Apr 3, 2012

recipes/default.rb
-directory node['activemq']['home'] do
- recursive true
+case node[:platform]
+when 'mac_os_x'
+ activemq_home = "#{node['activemq']['home']}/activemq/#{version}/libexec"
+ pidfile = "#{activemq_home}/data/activemq.pid"
+ platform = 'macosx'
@jhowarth

jhowarth Apr 3, 2012

Member

Why do we need to set this platform variable when node[:platform] is available everywhere?

@capoferro

capoferro Apr 3, 2012

Owner

activemq expects it in the format 'macosx' where ohai reports it as 'mac_os_x'

@jhowarth jhowarth commented on the diff Apr 3, 2012

recipes/default.rb
-file "#{activemq_home}/bin/activemq" do
- owner "root"
- group "root"
- mode "0755"
-end
+ unless File.exists?("#{activemq_home}/bin/activemq")
@jhowarth

jhowarth Apr 3, 2012

Member

What happens when we want to update the version?

Member

jhowarth commented Apr 3, 2012

👍 Mostly nitpicks on stuff you probably didn't even write.

Style of the if statement here might be corrected by opscode if merged in. Generally we use ternary operators for one liners and the else / end isn't tabbed in to match the if.

The most common style in cookbooks is to write it this way:

case platform
when "mac_os_x"
  default["activemq"]["home"] = "/usr/local/Cellar"
else
  default["activemq"]["home"] = "/opt"
end

We might want to specify ~> 10.6.0 or what we know it actually works against

another style note. Usually seen like this:

variables(
  :pidfile => pidfile
  :platform => platform
)

style here:

unnecessary new lines

reset commented Apr 3, 2012

Just some style nit picks. Everything else looks good!

@capoferro capoferro added a commit that referenced this pull request Apr 3, 2012

@capoferro capoferro Merge pull request #1 from RiotGames/os_x_support
Os x support
3fb4bb2

@capoferro capoferro merged commit 3fb4bb2 into master Apr 3, 2012

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