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

Test Solidus against Ruby 3.2 #4817

Merged
merged 1 commit into from
Jan 18, 2023
Merged

Conversation

kennyadsl
Copy link
Member

@kennyadsl kennyadsl commented Dec 27, 2022

Fixes #4814.

Summary

🎅 brought us a new Ruby version. Is Solidus ready for it?

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

  • I have written a thorough PR description.
  • I have kept my commits small and atomic.
  • I have used clear, explanatory commit messages.

The following are not always needed (cross them out if they are not):

  • I have added automated tests to cover my changes.
  • I have attached screenshots to demo visual changes.
  • I have opened a PR to update the guides.
  • I have updated the README to account for my changes.

@kennyadsl kennyadsl added type:enhancement Proposed or newly added feature changelog:solidus Changes to the solidus meta-gem labels Dec 27, 2022
@kennyadsl
Copy link
Member Author

We need to wait for CircleCI-Public/cimg-ruby#105 to have the Ruby 3.2 docker image available.

@jarednorman
Copy link
Member

jarednorman commented Dec 31, 2022

A lot of the errors currently look like they relate to keyword handling. Should be easy enough to fix.

CleanShot 2022-12-31 at 08 59 43

@github-actions github-actions bot removed the changelog:solidus Changes to the solidus meta-gem label Jan 6, 2023
@kennyadsl kennyadsl added the changelog:solidus Changes to the solidus meta-gem label Jan 11, 2023
@kennyadsl kennyadsl marked this pull request as ready for review January 16, 2023 06:26
@kennyadsl kennyadsl requested a review from a team as a code owner January 16, 2023 06:26
@github-actions github-actions bot removed the changelog:solidus Changes to the solidus meta-gem label Jan 16, 2023
@waiting-for-dev waiting-for-dev added the changelog:repository Changes to the repository not within any gem label Jan 16, 2023
@waiting-for-dev
Copy link
Contributor

A lot of the errors currently look like they relate to keyword handling. Should be easy enough to fix.

CleanShot 2022-12-31 at 08 59 43

@jarednorman, can you still see those errors? 🤔

@kennyadsl
Copy link
Member Author

can you still see those errors? 🤔

@waiting-for-dev they've been solved when the new version of rspec-mocks has been released, see this comment.

Copy link
Contributor

@waiting-for-dev waiting-for-dev left a comment

Choose a reason for hiding this comment

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

Thanks, @kennyadsl ❤️

@kennyadsl kennyadsl self-assigned this Jan 18, 2023
Copy link
Member

@elia elia left a comment

Choose a reason for hiding this comment

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

All great! Just one questions, not blocking

@@ -334,7 +334,7 @@ workflows:
- test_solidus:
context: slack-secrets
name: &name "test-rails-<<matrix.rails>>-ruby-<<matrix.ruby>>-<<matrix.database>>-<<#matrix.paperclip>>paperclip<</matrix.paperclip>><<^matrix.paperclip>>activestorage<</matrix.paperclip>>"
matrix: { parameters: { rails: ['7.0'], ruby: ['3.0', '3.1'], database: ['mysql', 'sqlite', 'postgres'], paperclip: [true, false] } }
matrix: { parameters: { rails: ['7.0'], ruby: ['3.1', '3.2'], database: ['mysql', 'sqlite', 'postgres'], paperclip: [true, false] } }
Copy link
Member

Choose a reason for hiding this comment

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

Thoughts on keeping 3.0 + rails 7 around?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can, I just didn't want to add more time for the CI (it's three more steps with that). Do you think it would be better?

Copy link
Member

Choose a reason for hiding this comment

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

I think it still would be better, and ensure we have a ruby version tested for adjacent rails versions, e.g. having ruby 3.0 be the shared version between rails 7 and 6.1, but it's just a nice to have.

It's fine if we merge and then add it later 👍👍👍

Copy link
Member Author

@kennyadsl kennyadsl Jan 18, 2023

Choose a reason for hiding this comment

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

Ok, that makes a lot of sense. I think we might accomplish the same result by testing Ruby 3.1 compatibility with Rails 6.1. This way, the CI will have only one more job instead of three new ones.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

This commit changes the CI matrix a little bit:

The main tests are performed against Rails 7 vs Ruby 3.1 and 3.2.

Testing Ruby 3.1 has been added to the Rails 6.1 matrix in order
to guarantee at least one overlapping Ruby version between Rails
version. This way there's a safe path to upgrade from
Rails 6.1/Ruby 2.7 up to the latest Rails 7.0/Ruy 3.2.
@github-actions github-actions bot removed the changelog:repository Changes to the repository not within any gem label Jan 18, 2023
@kennyadsl kennyadsl added the changelog:repository Changes to the repository not within any gem label Jan 18, 2023
@kennyadsl kennyadsl merged commit cdb4efc into solidusio:master Jan 18, 2023
@kennyadsl kennyadsl deleted the kennyadsl/ruby-3.2 branch January 18, 2023 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:repository Changes to the repository not within any gem type:enhancement Proposed or newly added feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Ruby 3.2
5 participants