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

Skip runit installation if already installed #62

Closed

Conversation

tdooner
Copy link

@tdooner tdooner commented Jul 25, 2014

If there is an executable at the desired installation location, we can assume that runit is already installed. Unfortunately, I can't find a way to verify that the correct version is installed, but given that the version isn't configurable via attributes, it seems reasonable that when/if there ever is a new version of runit, this logic can be changed.

I don't know how to test this effectively, give the current minitest-based suite. If you have any thoughts I'd be happy to write some tests.

Cheers!

If there is an executable at the desired installation location, we can
assume that runit is already installed. Unfortunately, I can't find a
way to verify that the *correct version* is installed, but given that
the version isn't configurable via attributes, it seems reasonable that
when/if there ever is a new version of runit, this logic can be changed.
@jtimberman
Copy link
Contributor

Checking that a file is executable isn't very reliable to determine if something is installed. I could touch /sbin/runit ; chmod +x /sbin/runit and this would then not work.

It would be better to move the rpm_installed to a predicate helper method (#rpm_installed?). It would also be better for the bash resource to not perform the rpm installation, and instead use another library helper to determine where the rpm_root_dir is, and use that for the install path, using rpm_package to perform the actual package installation. The bash resource could use creates with the path to the RPM, or it could use the predicate method I'm suggesting.

@tdooner
Copy link
Author

tdooner commented Jul 26, 2014

Sorry, let me clarify. My problem isn't with the rpm detection, it's that I'm using Chef to converge an image that already has runit installed from source. I.e. there's no way to tell via rpm commands that runit is already installed.

Most Chef resources have a way to detect if they are already fulfilled by the host OS, and if so, skip doing anything too complex. But, this cookbook will download the source for runit and compile it on rhel systems if node['runit']['use_package_from_yum'] is false. For example, some other cookbooks that do this kind of check:

https://github.com/opscode-cookbooks/activemq/blob/master/recipes/default.rb#L31
https://github.com/miketheman/nginx/blob/master/recipes/source.rb#L106,L109
https://github.com/fnichol/chef-rbenv/blob/0a3018634bafe58ad21c6ee271af015220e444b9/providers/global.rb#L31

Ultimately, this is just about saving the effort of downloading + building runit every time we converge a new image even though it is already installed on our base image. Not the worst cost, but it would be nice to avoid.

Would you be more open to this if it were an attribute, e.g. default['runit']['skip_install']? Or maybe there is another way to accomplish this?

(Oh, and thanks for the speedy reply!)

@jtimberman
Copy link
Contributor

Ah. Thanks for clarifying. I have some ideas... I'll make a branch.

@jtimberman
Copy link
Contributor

See the commit message comments in 1d6b6af for an explanation of the helper methods added. Does that work for you?

@tdooner
Copy link
Author

tdooner commented Jul 31, 2014

Beautiful, thanks a lot for putting that together. I'll give it a try today and I imagine it will do the trick.

@jtimberman
Copy link
Contributor

Wow, I totally forgot about this. Sorry about that. Did the helpers from my branch work out for you, @tdooner?

@slyness
Copy link

slyness commented Mar 9, 2015

Is this something that we're ready to merge or is there outstanding tasks or discussion? @jtimberman @tdooner

@jtimberman
Copy link
Contributor

@slyness I think the helper methods will be more reliable than simple checking if the executable is installed. I created pull request #89 as a proposal to replace this PR.

@tdooner
Copy link
Author

tdooner commented Mar 10, 2015

Sounds good. I'll close this in favor of #89. Thanks @jtimberman for addressing this.

@tdooner tdooner closed this Mar 10, 2015
@slyness slyness removed the question label Mar 13, 2015
jtimberman pushed a commit that referenced this pull request Mar 13, 2015
If a user is installing runit from source in a base image, as brought
up in #62, they probably want to skip the build and installation of
the package. We detect that runit is already installed in a robust
way:

* if the rpm is already installed, keeping the logic we had already
* if the `runit` executable is actually executable, AND the `sv`
  command works (`sv` will return 100 when run with `--help` and have
  usage information on STDERR.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants