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

Add specs #4

Merged
merged 18 commits into from
Mar 5, 2019
Merged

Add specs #4

merged 18 commits into from
Mar 5, 2019

Conversation

Fryguy
Copy link
Member

@Fryguy Fryguy commented Feb 21, 2019

Built on #3

Closes #1

@Fryguy Fryguy changed the title Add specs [WIP] Add specs Feb 21, 2019
@Fryguy Fryguy force-pushed the add_specs branch 5 times, most recently from a3e81f0 to 9a88754 Compare February 21, 2019 03:48
@Fryguy
Copy link
Member Author

Fryguy commented Mar 2, 2019

@jrafanie Here's what I came up with


Still have to write specs for

  • calling bundle check after the bundle update
  • calling bundle exec after the bundle update

@Fryguy Fryguy force-pushed the add_specs branch 2 times, most recently from 4dbc99b to 09f1b0d Compare March 4, 2019 17:40
@Fryguy Fryguy force-pushed the add_specs branch 3 times, most recently from 152b8cc to fddaf9f Compare March 4, 2019 17:53
@jrafanie
Copy link
Member

jrafanie commented Mar 4, 2019

This looks really good... 👍

@Fryguy Fryguy changed the title [WIP] Add specs Add specs Mar 4, 2019
@Fryguy
Copy link
Member Author

Fryguy commented Mar 4, 2019

@jrafanie This is ready to go.

@Fryguy
Copy link
Member Author

Fryguy commented Mar 4, 2019

or @NickLaMuro :)

@NickLaMuro
Copy link
Member

@Fryguy Would you be surprised by the fact that I have already started a review?

@Fryguy
Copy link
Member Author

Fryguy commented Mar 4, 2019

LOL nope 😂

@agrare
Copy link
Member

agrare commented Mar 4, 2019

Tested the following scenarios with ManageIQ/manageiq#18461 applied:

  1. Picked up global ~/.bundler.d when run from manageiq repo on initial run of bundle update
  2. Added override to manageiq/bundler.d then ran bundle update a second time, both global and local overrides present, bundle check successful
  3. Switched to a plugin manageiq-providers-vmware that had spec/manageiq symlinked, bundle update picked up overrides and globals
  4. Added local plugin override, bundle update picked up global, core, and plugin overrides, bundle check successful.

Any of the previous failures that we've seen are addressed from my point of view.

Just FYI

bundler --version
Bundler version 1.17.3
ruby --version
ruby 2.5.3p105 (2018-10-18 revision 65156) [x86_64-linux-gnu]

👍 from me

- TEST_BUNDLER_VERSION=1.15
- TEST_BUNDLER_VERSION=1.16
- TEST_BUNDLER_VERSION=1.17
- TEST_BUNDLER_VERSION=2.0
Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't use BUNDLER_VERSION, because bundler itself uses that internally (it sets it after it reads the lockfile, or you can specify to override the lockfile), so instead I went with this more explicit env var name.

Copy link
Member

@NickLaMuro NickLaMuro left a comment

Choose a reason for hiding this comment

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

Well, this is a start. Still need to make my way through the specs themselves, but this is what I noticed while looking through the helper code.

spec/spec_helper.rb Outdated Show resolved Hide resolved
spec/support/helpers.rb Outdated Show resolved Hide resolved
spec/support/helpers.rb Show resolved Hide resolved
spec/bundler/inject_spec.rb Show resolved Hide resolved
spec/support/helpers.rb Show resolved Hide resolved
@Fryguy
Copy link
Member Author

Fryguy commented Mar 4, 2019

@NickLaMuro Updated

Copy link
Member

@NickLaMuro NickLaMuro left a comment

Choose a reason for hiding this comment

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

Alright, I think I am fine with everything here.

Left a few minor nitpicks and commentary, but I wouldn't call any of it "merge blocking".

Looks good! 👍

spec/bundler_inject_spec.rb Outdated Show resolved Hide resolved
spec/bundler_inject_spec.rb Show resolved Hide resolved
spec/bundler_inject_spec.rb Show resolved Hide resolved
spec/bundler_inject_spec.rb Show resolved Hide resolved
spec/helpers_spec.rb Show resolved Hide resolved
spec/support/helpers.rb Show resolved Hide resolved
spec/support/helpers.rb Outdated Show resolved Hide resolved
end

puts
puts "Using bundler #{Spec::Helpers.bundler_version}".light_yellow
Copy link
Member

Choose a reason for hiding this comment

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

Interestingly, this doesn't show up as light_yellow on Travis.

@Fryguy
Copy link
Member Author

Fryguy commented Mar 5, 2019

@NickLaMuro Updated. Thanks for the review!

@NickLaMuro NickLaMuro merged commit 0db6d3a into ManageIQ:master Mar 5, 2019
@Fryguy Fryguy deleted the add_specs branch March 5, 2019 02:04
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