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

Fix ci/rubyonrails bugs, using bundle exec #2302

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

jamiemccarthy
Copy link

@jamiemccarthy jamiemccarthy commented Feb 12, 2024

This PR fixes three separate issues I identified with the rubyonrails workflow in #2159:

  1. Most seriously, the three linting commands must be run with bundle exec as a prefix, or they simply do not work. As written, the first lint command fails with bin/bundler-audit: No such file or directory. So this starter workflow has been broken for some time. This PR fixes that problem.
  2. bundle-audit is the preferred spelling of that command, as documented in its readme
  3. The setup-ruby readme notes: "Important: Prefer ruby/setup-ruby@v1" and this PR switches to pin to that tag instead of a commit. The readme explains "If you pin to a commit or release, only the Ruby versions available at the time of the commit will be available." The existing file, by pinning to a commit, has not only been missing ruby 3.3.0, but has required manual updates to fix CVEs, as in eeb9248. It's more secure to let the upstream maintainers pick up these fixes and trust them to keep v1 up to date. edit 2024-04-26: removed this change, see PR discussion for why

I'm happy to discuss these changes, and to reformat them in whatever way will be easiest. In particular, I've pushed the changes above as separate commits but I'm happy to squash them and resubmit.

I've been using a modified form of this workflow in a couple of jobs/workflows in a personal project since September. For what it's worth, it uses the bundle exec change, and it's been working fine.

Thank you!

@jamiemccarthy jamiemccarthy requested a review from a team as a code owner February 12, 2024 13:21
Copy link
Contributor

@jsoref jsoref left a comment

Choose a reason for hiding this comment

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

cc: @indirect, @bishal-pdMSFT, @JamesMGreene

fwiw, these changes look reasonable to me.

@indirect
Copy link
Contributor

You can create the expected binstubs by running bundle binstubs bundle-audit brakeman rubocop and then checking in the generated files. I personally use binstubs rather than bundle exec, because I like the discoverability of seeing what's inside bin/ in my application.

As for the ruby-setup version, I originally used v1 in my PR and was told that it is against policy for starter actions to be unpinned, and it was required to pin them to a precise hash for the PR to be accepted.

Good luck!

@jsoref
Copy link
Contributor

jsoref commented Apr 26, 2024

The pinning requirement is listed under Previous guidelines for new starter workflows:

### Previous guidelines for new starter workflows.
Before merging a new workflow, the following requirements need to be met:
- Should be as simple as is needed for the service.
- There are many programming languages and tools out there. Right now we don't have a page that allows for a really large number of workflows, so we do have to be a little choosy about what we accept. Less popular tools or languages might not be accepted.
- Automation and CI workflows should not send data to any 3rd party service except for the purposes of installing dependencies.
- Automation and CI workflows cannot be dependent on a paid service or product.
- We require that Actions outside of the `actions` organization be pinned to a specific SHA.

It doesn't appear to be the actual policy for this repository

organizations/users whose repositories aren't pinned
  • aliyun
  • aws-actions
  • azure
  • docker
  • github
  • google-github-actions
  • hashicorp
  • kentaro-m
  • microsoft
  • peter-evans
  • pozil
  • redhat-actions
  • slsa-framework
  • synopsys-sig
  • TencentCloud

I don't know what "Previous" means. It might mean "no longer in effect".

@jamiemccarthy
Copy link
Author

Ah, I see the requirement. Thanks for the heads-up, @indirect.

It's not in CONTRIBUTING.md, it's in the pull request template:

- [ ] This workflow must _only_ use actions that are produced by the language or ecosystem that the workflow supports. These actions must be [published to the GitHub Marketplace](https://github.com/marketplace?type=actions). We require that these actions be referenced using the full 40 character hash of the action's commit instead of a tag. Additionally, workflows must include the following comment at the top of the workflow file:

This workflow must only use actions that are produced by the language or ecosystem that the workflow supports. These actions must be published to the GitHub Marketplace. We require that these actions be referenced using the full 40 character hash of the action's commit instead of a tag.

This policy was adopted in 2021. At that time, there was discussion suggesting it shouldn't be a permanent restriction for some actions:

#806 (review)

You can use a qualified partners workflow too. And they don't need pinning to a 40 char SHA

Not sure how users will know which actions are from qualified partners... I'll merge this PR as-is for now, we can alway return to this and relax our conditions further in a future iteration.

I can see arguments either way, but that's out of scope for this PR.

So unless y'all object, I'll go ahead and push a commit that switches back from @v1 to the latest hash.

I think the rest of this PR is still worth doing. I don't much care whether it uses bundle binstubs to write out new files or execs them with bundle exec. As long as the result is that it works :)

@jamiemccarthy jamiemccarthy requested a review from a team as a code owner April 26, 2024 11:13
@jamiemccarthy jamiemccarthy changed the title Fix ci/rubyonrails bugs Fix ci/rubyonrails bugs, using bundle exec Jun 8, 2024
@jamiemccarthy
Copy link
Author

I don't have a strong preference for whether the ruby commands get run with bundle exec or as binstubs. This PR runs them with bundle exec. I've just pushed #2427 which runs them with binstubs.

I am proposing that one of them should be merged, depending on personal preference for how those ruby commands get run. The other PR should be closed. I don't care which 😄

@jamiemccarthy
Copy link
Author

@indirect @jsoref Just re-upping this request. This PR's been updated, and #2427 has also been submitted as an alternative that uses binstubs.

@jsoref
Copy link
Contributor

jsoref commented Jun 9, 2024

I have no influence over this repository. I have a project I want added to it, but I haven't even tried to submit it. I'm looking over existing PRs before I try to submit mine.

(I have made some PRs to bump generic dependencies, to test the waters, and it hasn't been particularly fast.)

@indirect
Copy link
Contributor

@jamiemccarthy I have gotten one PR merged here in the past. It took 3 months. Good luck!

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.

None yet

3 participants