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

Upgrade Rails to 7.1.1 #1324

Merged
merged 13 commits into from
Jan 26, 2024
Merged

Upgrade Rails to 7.1.1 #1324

merged 13 commits into from
Jan 26, 2024

Conversation

kartiki975
Copy link
Contributor

@kartiki975 kartiki975 commented Jan 26, 2024

  • Upgraded rails 7.1.1 for an internal Shopify ticket
  • Removed mini_racer gem; see the comments below for more details

.github/workflows/main.yml Outdated Show resolved Hide resolved
shipit-engine.gemspec Outdated Show resolved Hide resolved
@kartiki975
Copy link
Contributor Author

kartiki975 commented Jan 26, 2024

@casperisfine Post rails upgrade, mini_racer is breaking wherever Ruby version is less than 3.2 in the Github Actions. Looking at https://github.com/rubyjs/mini_racer?tab=readme-ov-file#supported-ruby-versions--troubleshooting, it seems like there is no workaround where the Ruby version is less than 3.2:

make sure you have Rubygems >= 3.2.13 and bundler >= 2.2.13 installed: gem update --system

Note, I have tried this with the existing and now, the latest version of mini_racer here: ba2c64a.

At this point, there two actions I can take:

  • upgrade Ruby to 3.2 which would mean I will have to ship the original PR I tagged you in
  • removing mini_racer; do you know if this will break other changes?

Thoughts?

@casperisfine
Copy link
Contributor

  • removing mini_racer; do you know if this will break other changes?

We can remove mini_racer and if nodejs is around, ExecJS will automatically use tha, it's probably better that way.

@kartiki975 kartiki975 changed the title Only upgrade rails Upgrade Rails to 7.1.1 Jan 26, 2024
@kartiki975 kartiki975 requested review from casperisfine and a team January 26, 2024 18:47
@kartiki975 kartiki975 marked this pull request as ready for review January 26, 2024 18:47
docs/setup.md Outdated Show resolved Hide resolved
Copy link
Contributor

@kwboyd-shopify kwboyd-shopify left a comment

Choose a reason for hiding this comment

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

This looks good to me but I'd aim for at least one more approval on it!

@kartiki975 kartiki975 requested a review from a team January 26, 2024 21:01
@kartiki975 kartiki975 merged commit 03a10e9 into main Jan 26, 2024
17 checks passed
@kartiki975 kartiki975 deleted the only-upgrade-rails branch January 26, 2024 21:19
@casperisfine
Copy link
Contributor

Thanks for taking care of the upgrade, LGTM.

However next time please try to keep a clean git history, wip and Add minor tweaks commit messages cause pain down the line when investigating the reason for a change.

@benlangfeld
Copy link
Contributor

Thanks for taking care of the upgrade, LGTM.

However next time please try to keep a clean git history, wip and Add minor tweaks commit messages cause pain down the line when investigating the reason for a change.

Worth setting the repo to force squash merges? Works wonders at my shop.

@casperisfine
Copy link
Contributor

Worth setting the repo to force squash merges?

It's one solution, but has its own downsides as some PR do legitimately contain more than one commit, e.g. a refactoring followed by a feature, etc.

Anyway, this was a general advice for contribution to all projects, not specific to shipit-engine.

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

6 participants