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

Enforced a minimum gem version of 1.0.4 for packagecloud-ruby #1292

Merged

Conversation

javabrett
Copy link
Contributor

@javabrett javabrett commented Jun 6, 2016

Tested by:

  • sudo gem uninstall packagecloud-ruby
  • sudo gem install packagecloud-ruby -v 1.0.2
  • sudo gem install packagecloud-ruby

... running ruby script/packagecloud.rb each time. You get the version warning until you hit 1.0.4, which is current. Could make this 1.0.3, the bare minimum, if preferred.

@javabrett
Copy link
Contributor Author

/cc #1288

begin
gem "packagecloud-ruby", ">=#{packagecloud_ruby_minimum_version}"
require "packagecloud"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a huge fan of using the exit code on L19 as control flow. How about moving stuff out of the block to look like this:

begin
  gem "packagecloud-ruby", ">=#{pkg_cloud_ruby_min_version}"
  require "packagecloud"
rescue LoadError
  puts "Requires ..."
  exit 1
end

require "packagecloud"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ttaylorr for the feedback. I am struggling a bit to apply it though.

Did you mean to move, rather than copy the require "packagecloud" statement, to be outside of the rescue block? I myself found the ruby mechanism for forcing the specific version or version minimum confusing here, but I have intentionally left both gem and require statements inside the begin/rescue/end block. The purpose of this block is to inform the developer that they don't have the required package/version, and fail-fast, preventing a deployment attempt. My understanding is that you cannot enforce the version with require, and gem will test that, but the require is still required.

So, I don't know whether gem could pass but require fail - this might be what you are angling at - that the require no longer needs to be in the block. I don't mind either way really - it looks OK to me now, but I'm no Ruby expert.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, huge oops on my end. Sorry about that, @javabrett.

It's fine as is, my suggestion should have omitted the last "require" statement outside of the begin/rescue block. I'm still not a huge fan of using the LoadError for control flow, so if you'd like, you could use Gem#latest_spec_for instead, but either works for me 👍

…is the current bare minimum). Avoids issues such as encountered in git-lfs#1288.

Installation of the gem is still typically done outside of the script and with sudo access for centralized installation.
@javabrett javabrett force-pushed the enforce-packagecloud-ruby-minimum-version branch from 817d1ad to 43ef114 Compare June 27, 2016 05:13
@ttaylorr
Copy link
Contributor

Looks good to me 👍 If you can merge the latest changes from upstream's master, I'd be happy to merge this in.

@ttaylorr ttaylorr merged commit 3ca6d04 into git-lfs:master Jun 28, 2016
@javabrett javabrett deleted the enforce-packagecloud-ruby-minimum-version branch June 30, 2016 00:42
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

2 participants