Skip to content

Switch to GitHub Actions for CI#124

Merged
sj26 merged 3 commits intobasecamp:masterfrom
sj26:github-actions
Aug 8, 2021
Merged

Switch to GitHub Actions for CI#124
sj26 merged 3 commits intobasecamp:masterfrom
sj26:github-actions

Conversation

@sj26
Copy link
Copy Markdown
Collaborator

@sj26 sj26 commented Aug 8, 2021

travis-ci.org is defunct and no longer running builds since June 2021. This pull request translates the existing build as directly as possible to GitHub Actions.

Many of these Ruby and Rails versions are now unsupported. Testing them is going to be nearly impossible. So I suggest merging this to demonstrate the versions which do work, and then modernising the version support to align with the Ruby maintenance policy and the Ruby on Rails maintenance policy.

Travis is defunct and no longer running builds on travis-ci.org for open
source since June 2021.

Translate the existing build to GitHub Actions first, then we can
freshen up the versions of ruby, rails, etc.
@sj26
Copy link
Copy Markdown
Collaborator Author

sj26 commented Aug 8, 2021

Here's this workflow running over in my fork:
https://github.com/sj26/marginalia/actions/runs/1109155953

It's pretty grim. The latest MySQL headers don't allow older versions of the mysql gems to compile, and the newer versions of the gem don't work with older ruby or rails.

Support ruby and rails versions do work. Rail 5.2 works on Ruby 2.4 through 2.7.

image

I'm a newcomer here, so I'll wait for a review. @jeremy and @byroot you seem the most recently active, how do you feel about merging this, and then trimming supported versions down to Rails 5.2+ and Ruby 2.6+ to match the maintenance policies?

@sj26 sj26 requested review from byroot and jeremy August 8, 2021 02:21
@sj26
Copy link
Copy Markdown
Collaborator Author

sj26 commented Aug 8, 2021

Trimming down to only supported ruby and rails versions gets the build passing:
sj26/marginalia@github-actions...remove-unsupported-versions
https://github.com/sj26/marginalia/actions/runs/1109191238

Adding all supported ruby and rails versions shows that rails 6.1 is broken:
sj26/marginalia@remove-unsupported-versions...modernise-versions
https://github.com/sj26/marginalia/actions/runs/1109180564

And rebasing my rails 6 branch (#91) on top should fix the build again, but it looks like there are more compatibility problems to fix:
sj26/marginalia@modernise-versions...rails-6
https://github.com/sj26/marginalia/actions/runs/1109188937

Comment thread .github/workflows/ci.yml Outdated
strategy:
fail-fast: false
matrix:
ruby-version: ["2.2", "2.3", "2.4", "2.5", "2.6", "2.7"]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Testing Ruby 3.0 is important because of the keyword argument semantic change.

Also not sure it's still worth testing super old rubies like 2.4.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Absolutely, did you see:

I suggest merging this to demonstrate the versions which do work, and then modernising the version support to align with the Ruby maintenance policy and the Ruby on Rails maintenance policy.

That would come next:

sj26/marginalia@remove-unsupported-versions...modernise-versions

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ah sorry, I didn't finish my coffee. It's up to you, but if it was me I'd go straight to the versions we actually care about.

Comment thread .github/workflows/ci.yml Outdated
fail-fast: false
matrix:
ruby-version: ["2.2", "2.3", "2.4", "2.5", "2.6", "2.7"]
gemfile: ["4.2", "4.2.api", "5.0", "5.1", "5.2"]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same here most of these Rails versions are totally EOL.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, again, I think we should only test 5.2, 6.0, and 6.1.

@byroot
Copy link
Copy Markdown
Collaborator

byroot commented Aug 8, 2021

Also are you aware of rails/rails#42240 ?

@sj26
Copy link
Copy Markdown
Collaborator Author

sj26 commented Aug 8, 2021

I wasn't aware of that PR, looks great!

Is there still some value to getting the build working and fixing marginalia for rails 6.1 as a stop gap until then?

@byroot
Copy link
Copy Markdown
Collaborator

byroot commented Aug 8, 2021

Is there still some value to getting the build working and fixing marginalia for rails 6.1

Sure. I was just suggesting not to invest more effort than it is worth.

@sj26
Copy link
Copy Markdown
Collaborator Author

sj26 commented Aug 8, 2021

Yeah, neat. That's kinda what I hoped, but I didn't want to be too ambitious.

How about this? Removes the old versions, and adds ruby 3 and rails 6. It's passing:
https://github.com/sj26/marginalia/actions/runs/1109840880

Rails 6.1 support will be added by #91.

The old guts to support previous versions of rails should be ripped out in the middle, I think. I have some commits to do that as another PR after this one, before rails 6.1 support.

I don't mind doing enough that using marginalia with rails 6.1 is production worthy. We currently use it in production, so I'd like to be sure it's well tested and stays compatible. Then if marginalia gets merged into rails and released in 7+ then this work can wither gracefully.

@byroot
Copy link
Copy Markdown
Collaborator

byroot commented Aug 8, 2021

👍

@sj26
Copy link
Copy Markdown
Collaborator Author

sj26 commented Aug 8, 2021

Great! Merged. I've also closed and tidied up the other related PRs.

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