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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use official Ruby setup GH action #8614

Merged
merged 5 commits into from
May 16, 2021
Merged

Conversation

seshrs
Copy link
Contributor

@seshrs seshrs commented Mar 24, 2021

This is a CI fix (housekeeping).

Summary

I noticed that the CI results in #8587 mentioned that actions/setup-ruby is no longer maintained and that we should migrate to using ruby/setup-ruby instead.

This PR updates jobs to use ruby/setup-ruby. I'm not super-familiar with Ruby and had to guess which steps were obsolete with the new action. Hopefully, the GH Actions results on this PR will help evaluate whether I got it right! 馃槄

EDIT: Haha that didn't work. Will try debugging later this week.

@seshrs
Copy link
Contributor Author

seshrs commented Mar 24, 2021

Oh I see some overlap with #8610. Will wait for that to merge first.

@MichaelCurrin
Copy link
Contributor

Looks good but I see there are conflicts to fix and this is WIP.

Copy link
Contributor

@MichaelCurrin MichaelCurrin left a comment

Choose a reason for hiding this comment

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

Fix conflicting file.

@seshrs
Copy link
Contributor Author

seshrs commented Mar 29, 2021

@MichaelCurrin yes you were right, the bundle env variables had to be declared at the job-level so that other steps use the correct bundle config! Thanks for pointing that out 馃槂

Given that the Third-Party Repository Profiling workflow completed successfully, I think we have more confidence that the docs workflow will also work when this PR is merged with master.

@seshrs seshrs marked this pull request as ready for review March 29, 2021 19:55
@MichaelCurrin
Copy link
Contributor

MichaelCurrin commented Mar 30, 2021

Yes good that the checks are passing.

And the job is setup to run on a PR against master and will work the same once merged.

@MichaelCurrin
Copy link
Contributor

You can find your branch marked with passing runs on the Actions tab

https://github.com/jekyll/jekyll/actions

@seshrs
Copy link
Contributor Author

seshrs commented Apr 21, 2021

Hi @MichaelCurrin, sorry I dropped the ball for a couple of weeks 馃槄

Is there some further action needed from me to push this PR forward?

@MichaelCurrin
Copy link
Contributor

No PR changes are outstanding. This just needs a review and merging by a maintainer - a reviewer with write access

@DirtyF DirtyF requested review from a team, mattr- and parkr and removed request for a team May 14, 2021 19:16
Copy link
Member

@parkr parkr left a comment

Choose a reason for hiding this comment

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

Looks good to me!

I wasn't able to fully validate the bundler-cache option but I'm assuming we'll be able to continue benefitting from the Actions cache.

@MichaelCurrin
Copy link
Contributor

MichaelCurrin commented May 15, 2021

More info on bundler cache flag here:

https://github.com/ruby/setup-ruby#single-job

That means that in this PR, we use the ruby/setup-ruby action to handle Bundle itself, bundle install step and cache of gems too. And so the cache action and the gem install steps are unnecessary and are good to be taken out.

@DirtyF
Copy link
Member

DirtyF commented May 16, 2021

@jekyll: merge +dev

@jekyllbot jekyllbot merged commit 56ef270 into jekyll:master May 16, 2021
@jekyllbot jekyllbot added the fix label May 16, 2021
jekyllbot added a commit that referenced this pull request May 16, 2021
@seshrs seshrs deleted the ruby-action-fix branch May 18, 2021 21:31
@jekyll jekyll locked and limited conversation to collaborators May 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants