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

Migrate from actions/setup-ruby to ruby/setup-ruby #5433

Merged
merged 1 commit into from
Aug 8, 2022

Conversation

jeffwidman
Copy link
Member

The actions/setup-ruby action was deprecated in favor of ruby/setup-ruby.

So this switches to that.

Reading the docs for ruby/setup-ruby, all the examples include the
following params:

with:
        ruby-version: '3.0' # Not needed with a .ruby-version file
        bundler-cache: true # runs 'bundle install' and caches installed gems automatically

However, the ruby-version looked like it needed to be specified for
actions/setup-ruby as well, and it wasn't, so maybe there's a default
that suffices?

And I suspect the bundler-cache arg is only relevant for repeated calls to ruby/bundler
within a single CI run... whereas we only call this action once for the following ruby call:

gem install rake && rake gems:release

So I doubt that caching adds value.
Happy to be proved wrong if someone more familiar with Ruby knows more.

@jeffwidman jeffwidman requested a review from a team as a code owner July 27, 2022 05:55
@jeffwidman jeffwidman changed the title The actions/setup-ruby action [was deprecated in favor of `ruby/set… Migrate from actions/setup-ruby to ruby/setup-ruby Jul 27, 2022
@jeffwidman jeffwidman force-pushed the switch-from-deprecated-setup-ruby-action branch from 369b8ef to 293302a Compare July 27, 2022 05:56
@jeffwidman
Copy link
Member Author

I'm not really sure how to test this... I have a funny feeling the only way is by cutting a release which should trigger this workflow to push the gem to rubygems...

@jeffwidman
Copy link
Member Author

@deivid-rodriguez can you please review this? In particular, see my comments on the PR description mentioning several args which I'm unclear whether should be included or not...

@jeffwidman
Copy link
Member Author

Actually, I wish there was an officially supported action for publishing to RubyGems... that would bypass a lot of what we've got here, no need to create a custom ~/.gem/credentials file etc...

I poked around, but all I could find were actions maintained by individuals, which I'm a bit leery of relying on for both security and stability concerns. Couldn't find anything maintained by Ruby / Ruby Gem organizations...

@deivid-rodriguez
Copy link
Contributor

Hi @jeffwidman!

I don't think you're using Bundler at all, here. If that's the case, you definitely don't need bundler-cache: true, and you don't even need Bundler at all so you could potentially specify bundler: none although I think that has effectively become the default in absence of a Gemfile.lock file, so I'd leave that alone.

And regarding automatic gem release action. Yes, there's nothing official yet. I think it's mainly because "automatic releases" required disabling two factor authentication or at least some complicated trickery to make it work with it. I think it would be a great idea to push for something fully secure and official, I will ask around!

@jeffwidman
Copy link
Member Author

jeffwidman commented Jul 27, 2022

Thank you for looking this over.

I think it's mainly because "automatic releases" required disabling two factor authentication or at least some complicated trickery to make it work with it.

Interesting. Looks like the current code here is already able to publish without 2FA though for some reason.

@deivid-rodriguez
Copy link
Contributor

Interesting indeed. Have a look at rubygems/rubygems.org#2500. There used to be the workaround of setting "UI only" as the MFA level, but that effectively means disabling MFA, and that will be deprecated because it's misleading. At the end of the linked thread there's another approach for automatically generating MFA codes that should be secure I think. But in any cases, a proper action would be great.

Copy link
Member

@Nishnha Nishnha left a comment

Choose a reason for hiding this comment

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

Maybe we should include

with:
    ruby-version: '2.7.6'

for this deploy and remove it in a future deploy since we don't know what the default version is and we don't have a .ruby-version file in repo

@deivid-rodriguez
Copy link
Contributor

deivid-rodriguez commented Jul 28, 2022

I think using the same version used elsewhere is a good idea 👍.

@jeffwidman
Copy link
Member Author

I'm actually hesitant to pin to a specific ruby version because it's so easy to go out of sync with our other code... for example, in #5432 I caught that we'd forgotten to bump a similar version pin.

And in order to merge #5030 we are likely going to bump to Ruby 3.1...

I'm not very knowledgable on ruby tooling, but I wonder... since this workflow is simply uploading the gem to rubygems, does it even matter what the Ruby version is? Ie, as long as it's a relatively recent version, won't the gem push succeed?
Or does the Ruby version have to match the gem version somehow?

This isn't something I feel strongly about, so if there's a reasonable case for pinning the version, I'm happy to do it, but thought I should raise these questions first...

@deivid-rodriguez
Copy link
Contributor

I don't think it has to match, specially here since as you explained is just about releasing gems. But I personally like to use the same fixed version everywhere. You're right that doing that would require extra work to not fall out of sync, but even if it falls out of sync, you're at least always using a fixed version for releasing and can be sure that a Ruby being released with some bug that affects this task can't ever affect it (I know it's very unlikely, but still).

@jeffwidman
Copy link
Member Author

I wonder if it might make more sense to specify a .ruby-version file to the repo? That would let us not only not pin here, but also Rubocop and other tooling would honor it: https://docs.rubocop.org/rubocop/configuration.html#setting-the-target-ruby-version

But this is a library, so we'd be tying the hands of our users, which we probably don't want to do...

Anyway, for now to get this across the line I'll pin it manually, and then have a separate conversation about pros/cons of switching to using .ruby-version...

@deivid-rodriguez
Copy link
Contributor

I think a .ruby-version file is a nice idea to keep things in sync more easily. And since it's not shipped with the library, it shouldn't force end users to use an exact Ruby version, which would be a bad idea indeed. The only thing that affects end users is the required_ruby_version field in gemspec, which should only set a lower bound for the Ruby version and we should be able to keep in sync easily through RuboCop too.

@jeffwidman
Copy link
Member Author

jeffwidman commented Aug 4, 2022

And since it's not shipped with the library, it shouldn't force end users to use an exact Ruby version

Oh interesting, didn't realize it wasn't shipped with the gem. In that case, we should probably use that. I'll opened #5483 to add that.

The `actions/setup-ruby` action [was deprecated in favor of `ruby/setup-ruby`](actions/setup-ruby@e932e7a#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R9).

So this switches to that.

Reading the docs for `ruby/setup-ruby`, all the examples include the
following params:
```
with:
        ruby-version: '3.0' # Not needed with a .ruby-version file
        bundler-cache: true # runs 'bundle install' and caches installed gems automatically
```

However, the `ruby-version` looked like it needed to be specified for
`actions/setup-ruby` as well, and it wasn't, so maybe there's a default
that suffices?

And I suspect the `bundler-cache` arg is only relevant for repeated calls to ruby/bundler
within a single CI run... whereas we only call this action once for the following ruby call:
```
gem install rake && rake gems:release
```

So I doubt that caching adds value.
Happy to be proved wrong if someone more familiar with Ruby knows more.
@jeffwidman jeffwidman force-pushed the switch-from-deprecated-setup-ruby-action branch from a7a5620 to befa7ff Compare August 4, 2022 17:06
@jeffwidman
Copy link
Member Author

I discussed with @Nishnha on slack, and he was also in favor of using .ruby-version file rather than pinning in the action, so going to merge w/o an explicit Ruby version pin in this action and rely on #5483.

@jeffwidman jeffwidman mentioned this pull request Aug 4, 2022
jeffwidman added a commit to jeffwidman/dependabot-core that referenced this pull request Aug 7, 2022
We're starting to pin the desired ruby version in multiple places:
* Rubocop
* the new `ruby/setup-ruby` action that we're adding in dependabot#5433

We used to not ship the `.ruby-version` file, with the thought that this
is a library... but this file isn't included in the gemfile spec, so
we're not tying the hands of our users. This merely sets the default
version of Ruby that we're using for development. We will keep this in
sync with the version of Ruby installed in the Dockerfile, so it'll
target the version of Ruby we use in production at GitHub. But since not
included in the gem, others are free to use a different Ruby version.

Copy the `.ruby-version` file into the CI container.
@jeffwidman jeffwidman merged commit ef77321 into main Aug 8, 2022
@jeffwidman jeffwidman deleted the switch-from-deprecated-setup-ruby-action branch August 8, 2022 02:48
@jurre jurre mentioned this pull request Aug 8, 2022
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.

3 participants