Skip to content

Audit and organize dependencies #148

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

Merged
merged 8 commits into from
Nov 9, 2016

Conversation

maxjacobson
Copy link
Contributor

  • move development dependencies to Gemfile
  • use bundler's platform directive to specify ruby version dependency
    differences
  • promote SimpleCov to a runtime depenency

Inspired by:

@codeclimate/review @dblandin

gem "rake"
gem "rspec"
gem "webmock"
gem "pry"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer to keep these dependencies within the gemspec.

Ideally, this file would only contain two lines:

source "http://www.rubygems.org"

gemspec

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I didn't see this thread before commenting: a0e5d03#commitcomment-19735625

Still thinking about how to manage this...

@maxjacobson maxjacobson force-pushed the mj-update-dependency-organization branch 5 times, most recently from e1ce959 to 2b45306 Compare November 9, 2016 21:04
@maxjacobson
Copy link
Contributor Author

@dblandin this is ready for another look

@@ -0,0 +1,19 @@
#!/bin/bash --login
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think of using the more portable #!/usr/bin/env bash shebang form?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like it, updating

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, it didn't work: https://circleci.com/gh/codeclimate/ruby-test-reporter/133 and I undid it. I think the loss in portability is a thing, but not a huge thing, because we don't change CI services that often... WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Works for me. Why is --login needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rvm use didn't work without it in an earlier permutation. Just double checked: https://circleci.com/gh/codeclimate/ruby-test-reporter/135 and still think it's needed for the ruby switching to work

Copy link
Contributor

Choose a reason for hiding this comment

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

Weird rvm. Cool cool.

bundle install --gemfile Gemfile.ruby-19
bundle exec rake

set +x
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we specify set -ex at the top instead of switching back and forth?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed it's a bit awkward. rvm use prints a ton of output (sample build: https://circleci.com/gh/codeclimate/ruby-test-reporter/130) so it seemed worthwhile to do this toggling dance to make the output more readable

Could just do set -e without the x, but I think we want it to make it more clear which step failed.... WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, that is a lot of output. I might just leave debug off then. We'll still get the stderr output, if there is any.

@dblandin
Copy link
Contributor

dblandin commented Nov 9, 2016

A couple minor comments. LGTM otherwise.

@maxjacobson maxjacobson force-pushed the mj-update-dependency-organization branch from 2b45306 to 5fe6fdd Compare November 9, 2016 21:24
* Promote SimpleCov to a runtime depenency
* Extract ruby 1.9 dependencies to separate Gemfile

Inspired by:

* a0e5d03#commitcomment-19735625
* #146 (comment)
@maxjacobson maxjacobson force-pushed the mj-update-dependency-organization branch from 5fe6fdd to 43b167a Compare November 9, 2016 21:26
hopefully when tests fail it will be clear from stderr output which ruby
versions suite failed.

I may need to return --login... some earlier rvm output encouraged me to
switch from

  #!/bin/sh

to

  #!/bin/bash --login

But maybe bash would've been sufficient
@maxjacobson maxjacobson merged commit 3252bb7 into master Nov 9, 2016
@maxjacobson maxjacobson deleted the mj-update-dependency-organization branch November 9, 2016 22:12
@andrew
Copy link

andrew commented Nov 9, 2016

👌

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.

3 participants