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

Use ruby/setup-ruby Github action #382

Merged
merged 2 commits into from
Jan 18, 2021
Merged

Conversation

ekohl
Copy link
Contributor

@ekohl ekohl commented Jan 4, 2021

The ruby/setup-ruby action is actually maintained by the Ruby people themselves. It has built in caching with the proper behavior. Not only is it easier to read (less syncing going on), it's also likely to be maintained properly. For example, to get proper caching of gems, bundle lock should run to get a Gemfile.lock which gives you a correct cache. This action does it for you.

Note I didn't test it myself since I don't use PDK, but we've been using this in Voxpupuli (https://github.com/voxpupuli/modulesync_config/blob/master/moduleroot/.github/workflows/ci.yml.erb) and it works well in my experience. The only thing we don't currently do is to run on push as well. That means we don't have a shared cache ready and caches are now scoped to a user. It looks like pdk-templates is in the same setup, but due to the nightly tests it probably gets away with this.

@ekohl ekohl requested review from a team as code owners January 4, 2021 10:58
@DavidS
Copy link
Contributor

DavidS commented Jan 4, 2021

Nice find!

This would also need to be added to the second time this sets up ruby at

(and the same for pr_test.yml.erb).

Given this section from ruby/setup-ruby:

When there is no lockfile, one is generated with bundle lock, which is the same as bundle install would do first before actually fetching any gem. In other words, it works exactly like bundle install. The hash of the generated lockfile is then used for caching, which is the only correct approach.

it does likely lead to more accurate caching, too.

The ruby/setup-ruby action is actually maintained by the Ruby people
themselves. It has built in caching with the proper behavior. Not only
is it easier to read (less syncing going on), it's also likely to be
maintained properly. For example, to get proper caching of gems, bundle
lock should run to get a Gemfile.lock which gives you a correct cache.
This action does it for you.
@ekohl
Copy link
Contributor Author

ekohl commented Jan 4, 2021

Something like this? I'm not sure what steps you want to log so do review that part.

This deploys @ekohl's original change to all the place where it needs to be
and rejiggers the surrounding actions and honeycomb labels to match up again.
@DavidS
Copy link
Contributor

DavidS commented Jan 5, 2021

I've updated the PR with my thoughts:

  • use ruby/setup-ruby consistently everywhere
  • record bundler env on the setup matrix job too
  • deploy changes to both PR and nightly job
  • rejig some of the labels around the change to make more sense

While you're at it, would you mind reading through the changes in #377 ? I'd love to hear you opinion on that one, too.

@@ -92,42 +106,22 @@ jobs:
uses: actions/checkout@v2

- name: Activate Ruby 2.7
uses: actions/setup-ruby@v1
uses: ruby/setup-ruby@v1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Since the meat of the two files is the same, I took on the habit of fixing one, and then copying over everything to the other (except the preamble and the slack notification) 🤷

can't wait for the day when actions support higher-level re-use - I don't want to re-compose all of this into a singular action just for this...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can't wait for the day when actions support higher-level re-use - I don't want to re-compose all of this into a singular action just for this...

Oh yes, absolutely. Templating and copying over to many repos is painful.

@DavidS DavidS merged commit 6b2e1e7 into puppetlabs:main Jan 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants