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

Add Percona repos at compile time for compatibility with chef_gem #75

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@klevo

klevo commented Jul 10, 2013

For example chef_gem mysql requires us to have percona-server-client installed at compile time - this can only happen if the percona repositories are there as well.

Robert Starsi
Percona repositories are added at compile time, due to `chef_gem` com…
…mand installing gems at compile time. For example `chef_gem mysql` requires us to have percona-server-client installed at compile time - this can only happen if the percona repositories are there as well.
@patcon

This comment has been minimized.

Contributor

patcon commented Jul 10, 2013

Hey @klevo, good catch!

We're currently in the process of having the percona cookbook leverage much more of the mysql cookbook, which handles this like so:
https://github.com/opscode-cookbooks/mysql/blob/master/recipes/ruby.rb

Think we could do something similar in the meantime? Perhaps a percona::ruby recipe for now? Thoughts @phlipper?

node.set['build_essential']['compiletime'] = true
include_recipe "build-essential"
include_recipe "percona::client"

case node['platform_family']
when "debian"
  # apt-related stuff, ie resource("apt_preference[00percona]").run_action(:add) etc
when "rhel"
  # yum-related stuff
end

node['mysql']['client']['packages'].each do |mysql_pack|
  resources("package[#{mysql_pack}]").run_action(:install)
end

chef_gem "mysql"

I know it's more verbose, but running resources at compile-time is pretty opinionated, and most people likely don't need it :)

@phlipper

This comment has been minimized.

Member

phlipper commented Jul 10, 2013

@klevo thanks for this, I really appreciate it!

I hate to be a stickler, but could I trouble you to update the style just a bit? I'm really not a fan of end.method_call-style chaining. I'd much rather see something like:

r = some_resource do
  #
end
r.run_action(:add)

I'm ok with @patcon's suggestion of a percona::ruby recipe for now if that works for everyone.

Robert Starsi
@klevo

This comment has been minimized.

klevo commented Jul 10, 2013

I hate to be a stickler, but could I trouble you to update the style just a bit?

Done.

As for percona::ruby recipe - I don't see the necessity as it would just copy what mysql::ruby does, which now works well together with Percona thanks to the above commits. My use case for my patch was using Opscode's databases cookbook which does include_recipe 'mysql::ruby'.

@patcon

This comment has been minimized.

Contributor

patcon commented Jul 10, 2013

Coolio. On further consideration, percona::ruby should be as simple as this:

include_recipe "percona::package_repo"

case node['platform_family']
when "debian"
  resource("apt_preference[00percona]").run_action(:add)
  resource("apt_repository[percona]").run_action(:add)
when "rhel"
  resource("yum_key[RPM-GPG-KEY-percona]").run_action(:add)
  resource("yum_repository[percona]").run_action(:add)
end

include_recipe "mysql::ruby"

I totally understand if it's not a necessity for your fix, but I suppose I feel that opscode had the right approach with separating out concerns and creating a distinct cookbook when forcing compile-time actions. If it's a matter of not having time, and you guys aren't opposed to the premise, I'm totally happy to roll a new PR @klevo. Just lemme know man :)

@klevo

This comment has been minimized.

klevo commented Jul 10, 2013

The downside of a separate recipe is that it would have to be included in the run_list, to achieve the desired interoperability with other cookbooks that use chef_gem. It's little bit less obvious - in fact it took quite some time to debug the problem to chef_gem, repos and compile time. So README would have to say something like: "If you're using other cookbooks that install chef_gem 'mysql' add percona::ruby to your runlist." Unless of course you intend for this recipe to be included in ::default.

The current solution seems to me to be "just works out of the box".

@patcon

This comment has been minimized.

Contributor

patcon commented Jul 10, 2013

This precedent would be no different from that adopted by the mysql cookbook. Opscode even lists it as an anti-pattern, so I'm inclined to avoid it where possible.

@klevo

This comment has been minimized.

klevo commented Jul 10, 2013

cool, that explains it - thanks for the education :) If you don't mind then, roll a new PR.

@patcon

This comment has been minimized.

Contributor

patcon commented Jul 10, 2013

Thanks dude. Mind if I credit you as the author? The hard part is tracking down the bugs anyhow :)

@klevo

This comment has been minimized.

klevo commented Jul 11, 2013

Mind if I credit you as the author?

sweet 👍

@patcon

This comment has been minimized.

Contributor

patcon commented Jul 11, 2013

Cool. The test suite is actually failing, and I spent a little too much time on it yesterday. When I get a chance, I'll see if it passes even with the working minimum viable solution you'd offered :)

Will like be next week before I have time to fix the tests. Feel free to reopen and merge if that's too slow, and we can always create a new PR later. (Maybe that was the best solution all along!)

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