-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Lock Sprockets to v3.x #3378
Lock Sprockets to v3.x #3378
Conversation
3143a1b
to
b4c7500
Compare
Gemfile
Outdated
@@ -8,6 +8,13 @@ group :backend, :frontend, :core, :api do | |||
rails_version = ENV['RAILS_VERSION'] || '~> 6.0.0' | |||
gem 'rails', rails_version, require: false | |||
|
|||
if ENV['CIRCLE_TEST_REPORTS'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious about CIRCLE_TEST_REPORTS
var: why not using one of the built-in env flags: https://circleci.com/docs/2.0/env-vars/#built-in-environment-variables ? 🤔(CI
or CIRCLECI
, both booleans).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't know exactly what env variables were set in circleCi by default, so I went with one that's explicitly set in the file circle.yml
After looking at the list I think the best candidate is CI
, wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree 👍 I usually see ENV['CI']
used in these cases
b4c7500
to
9f26124
Compare
@spaghetticode I'd lock it anyway (not on CI only), we have this failure even running tests locally and on extensions due to this issue. |
@kennyadsl locally one can use |
Temporary fix for solidusio#3374
9f26124
to
30270d0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Andrea!
@spaghetticode can you please rebase against upstream master to have Rails 5.1 specs out of the required checks? |
@kennyadsl it's already updated. I removed the requirements for Rails 5.1 branches by updating github configuration. PR is now ready to merge. |
Description
Ref #3374
This is a temporary fix that will allow master to be green again (so we can continue working on PRs) while we wait for rails/sprockets-rails#446 to be merged (or some other working solution).
Checklist: