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

Properly auto-install the right bundler version #168

Merged
merged 2 commits into from
Mar 26, 2020
Merged

Properly auto-install the right bundler version #168

merged 2 commits into from
Mar 26, 2020

Conversation

deivid-rodriguez
Copy link
Contributor

And fix failures.

@nickcharlton This is equivalent to #166, but I included a small fix in the test code, so we can use the latest version of bundler.

@nickcharlton
Copy link
Member

Interestingly, this command.rb fixes another problem I was trying to solve: #146 and I think is valuable on it's own.

I merged my Ruby update PR (#166) to split this from this PR and just rebased yours (hopefully to save you the bother and not just be annoying…).

It'd be a good to see a test here too. Do you think there's something we could do to ensure this is working?

@deivid-rodriguez
Copy link
Contributor Author

I was thinking that since this patch allows appraisals tests to pass against the latest version of bundler, actually testing against the latest version bundler would be enough to cover the fix (or maybe testing against both 1.17 and 2.1?).

If this needs a more specific test, then I'll try to find out some time to think about it.

@nickcharlton
Copy link
Member

Right yeah, I agree with that. I'm curious that whilst we're not currently testing this, it's different behaviour and whether or not we should or not.

Incidentally, I tried this branch on Ruby 2.7 on Administrate and we get some fun errors there: https://circleci.com/gh/thoughtbot/administrate/5904 any thoughts?

@deivid-rodriguez
Copy link
Contributor Author

I'm having a look now 👍

@deivid-rodriguez
Copy link
Contributor Author

deivid-rodriguez commented Feb 16, 2020

Ok, so this PR does fix the issue of auto installing the correct bundler version your CI wants to use (in the case of administrate, 1.17.3).

However, that reveals a potential incompatibility between bundler 1.17 and ruby 2.7 as the errors you reference show.

Upgrading bundler gets past that issue, but reveals yet another incompatibility, in this case between ruby 2.7 and rails 4.2.

You might be able to fix that last issue by pinning administrate to bigdecimal 1.x, but honestly I think the best way to "fix" it is by dropping Rails 4.2 support.

@deivid-rodriguez deivid-rodriguez changed the title Run tests against latest versions of supported rubies Properly auto-install the right bundler version Feb 17, 2020
@deivid-rodriguez
Copy link
Contributor Author

Any news @nickcharlton? :)

@nickcharlton
Copy link
Member

I think on further thought that this is the right approach to take, so I'm going to merge it and see what goes wrong!

@deivid-rodriguez
Copy link
Contributor Author

Thanks @nickcharlton!

@deivid-rodriguez deivid-rodriguez deleted the fix_warnings branch March 26, 2020 19:54
odlp added a commit to remove-bg/ruby that referenced this pull request May 20, 2020
odlp added a commit to remove-bg/ruby that referenced this pull request May 20, 2020
@odlp
Copy link

odlp commented Jun 2, 2020

@nickcharlton 👋 hello! Any chance of cutting a release with this?

@nickcharlton
Copy link
Member

👋 hello friend!

Yeah — I hadn't revisited since so I must've forgotten about it. I'll do this on Friday (2020-06-05).

tagliala added a commit to tagliala/appraisal that referenced this pull request Mar 28, 2021
Prevents `appraisal rake` to write a boolean on the standard output when
checking the installed version of bundler

Bug introduced in thoughtbot#168 5868643

Fix: thoughtbot#180
nickcharlton pushed a commit that referenced this pull request May 10, 2021
Prevents `appraisal rake` to write a boolean on the standard output when
checking the installed version of bundler

Bug introduced in #168 5868643

Fix: #180
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