Adds Rails 7.1.x and Ruby 3.3 to CI matrix #170
Conversation
berkos
changed the title
| @@ -6,13 +6,8 @@ jobs: | |||
| strategy: | |||
| fail-fast: false | |||
| matrix: | |||
| ruby: [ '2.7', '3.0', '3.1', '3.2', 'head', 'truffleruby' ] | |||
| rails: [ '6.1', '7.0', 'main' ] | |||
| include: | |||
There was a problem hiding this comment.
Just looking at this briefly, I don't understand what the purpose of this include is yet.
It was updated in #153 to remove all of the old versions, but curious as to why it was removed here if you have ideas.
There was a problem hiding this comment.
👋 I think, based on my assumption, but I could also be wrong that this was a left over from previous commits.
More specific:
The includes for Ruby 3.2 was added early here: 8b2fdd6 when there was still support for Rails 5.1 (which does not work with Ruby 3.2 and made sense 3.2 to be explicit).
Then later Rails 5.1 was removed but that include was still left there due to the reason that ruby: [...] did not had 3.2.
Then, when 3.2 was added in the main ruby: [...] ecc0adf which by that time there was no Rails 5.1 it would make sense to also remove this specific includes as there were not really adding any new matrix combination.
For the current PR here, I thought it would make sense to remove those extra includes since they are not adding any new jobs as it is.
Reference also on Github CI docs
There was a problem hiding this comment.
Just eyeballing the two workflows from this branch and main, I can't tell the difference 🤔
There was a problem hiding this comment.
Hi @zzak thanks for the response, I am not sure I am following.
Do you mean the actual workflows? 🤔
On main we have a total of 18 checks:
Which is 6 Ruby flavours * 3 Rails flavours
While on this branch:
7 Ruby flavours * 4 Rails flavours == 28 runs as It adds Rails 7.1 and Ruby 3.3
There was a problem hiding this comment.
Yeah, I mean not counting the new workflows you've added, it seems like there is no difference without the include which makes me think it's fine to be removed. 🤔
There was a problem hiding this comment.
Ah, yes! As I mentioned on my first response, based on how the main branch is now that include was not making any difference, just like you said, hence the cleanup.🙂
There was a problem hiding this comment.
Hi @zzak , please let me know if there any pending actions on this. 🙂
There was a problem hiding this comment.
I've given it a look and like I said, the workflows look fine so I don't think there is anything actionable, just wait for a review. 🙏
Maybe you could squash your commits?
There was a problem hiding this comment.
Awesome, commits look squashed ✨ !
berkos
force-pushed
the
add-rails-7.1-to-ci-matrix
branch
2 times, most recently
from
0ad9649 to
39f3431
Compare
- Update CI badge on Contributing.md
berkos
force-pushed
the
add-rails-7.1-to-ci-matrix
branch
from
39f3431 to
2db6cb7
Compare
👋 Thanks for the review in advance.
It also updates the CI badge that was pointing on Travis