search

tumblr / genesis

A tool for data center automation
http://tumblr.github.io/genesis/
Apache License 2.0
156 stars 24 forks source link

Gem install bug fixes #40

Closed defect closed 9 years ago

defect commented 9 years ago

This pull request fixes/changes two things:

1) When installing gems using the DSL, include the gem sources specified in the config. Right now we can only install gems coming from rubygems.

2) install :gem tries to automagically require the gems we just installed. However, the name of the gem and the name we need to require isn't always the same which makes the task fail. This changes it so that we print a message if we fail to require the gem, and lets the user require it in the task. I would like to nuke the autoloading completely, but i also don't want to break backwards compatibility :)

@roymarantz @maddalab @byxorna @Primer42

byxorna commented 9 years ago

LGTM, save for the more descriptive error message :)

roymarantz commented 9 years ago

I agree that doing a hidden require is a little smelly, but it is handy.

An alternative to breaking that and fixing all tasks would be to extend the syntax of install :gem such that the arguments can be a list of [gem, require] name pairs instead of a list of gem names only. i.e. install :gem, ['gem1', 'require1'], :gem2, .... or if you prefer a hash install :gem 'gem1' => 'require1', 'gem2' => 'gem2', ...

obviously we would need to be careful to parse: install :gem, 'gem1', 'gem2' correctly as install gem1 gem1 ; require gem1 gem2 and not as install gem1; require gem2

Bother these have there own amoung of smell too.

defect commented 9 years ago

@roymarantz I've added functionality to specify what to require to the install :gem DSL method now. This:

install :gem, 'gem1', 'gem2' => ['gem2/foo', 'gem2/bar']

will install gem1 and gem2 and require gem1, gem2/foo and gem2/bar.

@Primer42 I've also changed the name of gems_source to gem_args and added an example to the testenv example config.

roymarantz commented 9 years ago

I'm fine with this once the sample gem_args is fixed although I'd prefer the code change too. I really like this. Good work. :100:

defect commented 9 years ago

Thanks @roymarantz. Makes a lot of sense. I've fixed your comments, now go celebrate july 4th! :P

defect commented 9 years ago

This has been running happily in our production environment for a few days now. Unless anyone objects I'll merge this on monday.