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

v2.3.7 #861

Merged
merged 2 commits into from
Jan 11, 2022
Merged

v2.3.7 #861

merged 2 commits into from
Jan 11, 2022

Conversation

timothysmith0609
Copy link
Contributor

@timothysmith0609 timothysmith0609 commented Jan 10, 2022

What are you trying to accomplish with this PR?
We need to explicitly require active_support/isolated_execution_state, otherwise a NameError is raised (see image)

image

How is this accomplished?
Add the requirement where we load all the other active_support dependencies

rails/rails issue for more context

What could go wrong?
Could result in a similar error if we're not requiring everything we need, though that seems unlikely.

@timothysmith0609 timothysmith0609 requested a review from a team as a code owner January 10, 2022 16:22
@timothysmith0609 timothysmith0609 requested review from stefanmb and devonboyer and removed request for a team January 10, 2022 16:22
@gigr
Copy link
Contributor

gigr commented Jan 11, 2022

Also related: #851. @timothysmith0609 an aside but we can reproduce this bug on our local shipit instance, which has not yet been upgraded to Rails 7. ActiveSupport 6.1.4.4 causes the issue, but downgrading to 6.1.4.3 does not.

@timothysmith0609
Copy link
Contributor Author

Can we get rid of the conditional and just always include that sublibrary, then?

@gigr
Copy link
Contributor

gigr commented Jan 11, 2022

I dug a bit deeper and realized that while our shipit instance was running activesupport 6, the deploy task where we saw the failure was running activesupport 7. I think this is good to go.

@timothysmith0609 timothysmith0609 changed the title isolated_execution_state active_support require for Rails 7+ v2.3.7 Jan 11, 2022
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.

2 participants