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

Fixes #14460 - Add ability to use local Gemfiles #14

Merged
merged 1 commit into from
Apr 5, 2016
Merged

Fixes #14460 - Add ability to use local Gemfiles #14

merged 1 commit into from
Apr 5, 2016

Conversation

cpeters
Copy link
Contributor

@cpeters cpeters commented Apr 4, 2016

No description provided.

@theforeman-bot
Copy link
Member

There were the following issues with the commit message:

  • d170122 must be in the format fixes #redmine_number - brief description

If you don't have a ticket number, please create an issue in Redmine, selecting the appropriate project.

More guidelines are available on the Foreman wiki.


This message was auto-generated by Foreman's prprocessor

@stbenjam
Copy link
Member

stbenjam commented Apr 4, 2016

Oops, sorry, looks like we did turn on redmine issues for the repo.

@cpeters Can you file one please?

@cpeters cpeters changed the title Add ability to create a Gemfile.local to override gems Fixes #14460 - Add ability to create a Gemfile.local to override gems Apr 4, 2016
@cpeters
Copy link
Contributor Author

cpeters commented Apr 4, 2016

@stbenjam Updated

@stbenjam
Copy link
Member

stbenjam commented Apr 4, 2016

Shouldn't we do the same as Foreman and Smart Proxy and read a Gemfile.local.rb from bundler.d? Or is there somewhere else that the other repos support this Gemfile.local thing that I missed?

They seem to do it this way:

Dir["#{File.dirname(__FILE__)}/bundler.d/*.rb"].each do |bundle|
  self.instance_eval(Bundler.read_file(bundle))
end

@cpeters
Copy link
Contributor Author

cpeters commented Apr 4, 2016

@stbenjam The hammer cli plugins do it with a Gemfile.local file.

https://github.com/theforeman/hammer-cli-foreman-discovery/blob/master/Gemfile#L18-L19

I'm not opposed to doing it with bundler.d similar to Foreman

@stbenjam
Copy link
Member

stbenjam commented Apr 4, 2016

Ok cool, how about Foreman's style then?

@shlomizadok
Copy link
Member

+1 for Foreman style

@cpeters cpeters changed the title Fixes #14460 - Add ability to create a Gemfile.local to override gems Fixes #14460 - Add ability to use local Gemfiles Apr 4, 2016
@cpeters
Copy link
Contributor Author

cpeters commented Apr 4, 2016

@stbenjam @shlomizadok Updated

@stbenjam
Copy link
Member

stbenjam commented Apr 4, 2016

Oh, bundler.d is already used, the dynflow.rb is copied to the smart proxy dir using this path. It's the same in all the smart proxy plugins. It's causing an error to actually process gems from that dir, since it's including itself.

See packaging: https://github.com/theforeman/foreman-packaging/blob/rpm/develop/rubygem-smart_proxy_dynflow/rubygem-smart_proxy_dynflow.spec#L63

So I'm not sure, maybe we should adopt the hammer style.

@cpeters
Copy link
Contributor Author

cpeters commented Apr 4, 2016

Yeah. I tried modifying the gemspec, removing the bundler.d from gem.files and still ran into a bundler issue.

I also tried creating a bundler.d/smart-proxy-local.rb file and having the gemspec include it but it still fetched smart-proxy from upstream despite me pointing to a downstream git repo.

@stbenjam
Copy link
Member

stbenjam commented Apr 4, 2016

The packaging gets it from the gem, so it needs to stay.

@cpeters
Copy link
Contributor Author

cpeters commented Apr 4, 2016

@stbenjam Ok; back to the original with pulling Gemfile.local into the Gemfile.

@stbenjam
Copy link
Member

stbenjam commented Apr 4, 2016

Looks good to me, thanks! Will merge tomorrow, just want to give the others on @theforeman/remote-execution a chance to make sure it's ok for them.

@@ -6,3 +6,7 @@ group :development do
gem 'smart_proxy', :git => "https://github.com/theforeman/smart-proxy", :branch => "develop"
gem 'pry'
end

# load local gemfile
local_gemfile = File.join(File.dirname(__FILE__), 'Gemfile.local')
Copy link
Member

Choose a reason for hiding this comment

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

bikesheding: Gemfile.local.rb would be better, as editors recognize it's a ruby source.

Also adding it to .gitignore would make sense here

@iNecas
Copy link
Member

iNecas commented Apr 4, 2016

Thanks @stbenjam for waiting :)

@cpeters
Copy link
Contributor Author

cpeters commented Apr 4, 2016

@iNecas Updated

@iNecas
Copy link
Member

iNecas commented Apr 4, 2016

👍

@stbenjam
Copy link
Member

stbenjam commented Apr 4, 2016

I was inclined to also make the .rb comment, but didn't because we'll have 3 different ways of doing this in the Foreman org (bundler.d, Gemfile.local.rb, and Gemfile.local). 😿 https://xkcd.com/927/

Anyway, it's the right solution. I opened a RM against hammer. http://projects.theforeman.org/issues/14466

@stbenjam stbenjam merged commit 2ea8406 into theforeman:master Apr 5, 2016
@stbenjam
Copy link
Member

stbenjam commented Apr 5, 2016

Merging, thanks @cpeters, @iNecas

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.

5 participants