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

CI: add github action to block merging large files #57360

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

camdencheek
Copy link
Member

@camdencheek camdencheek commented Oct 4, 2023

This adds a github action which checks that the PR doesn't add any files greater than 1MB. We've had a lot of large binaries or test files be committed recently, so this should help block those from making it to main, since once they're in main, they're in the history permanently.

Test plan

See example failure here.

@cla-bot cla-bot bot added the cla-signed label Oct 4, 2023
@camdencheek camdencheek force-pushed the cc/test-notify branch 2 times, most recently from 6bf68b2 to e6a8009 Compare October 4, 2023 22:47
@camdencheek camdencheek changed the title Cc/test notify CI: add github action to block merging large files Oct 4, 2023
@camdencheek camdencheek marked this pull request as ready for review October 4, 2023 23:57
@camdencheek camdencheek requested review from a team and eseliger October 4, 2023 23:59
@camdencheek camdencheek marked this pull request as draft October 5, 2023 00:02
@camdencheek
Copy link
Member Author

Converted to draft because the skip logic doesn't seem to work

Copy link
Member

@eseliger eseliger left a comment

Choose a reason for hiding this comment

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

bash isn't my strength but if it works great!

Copy link
Member

@jhchabran jhchabran left a comment

Choose a reason for hiding this comment

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

It's great!

Do you mind adding a small snippet that comments on the PR as well? It's a rather low hanging fruit:

jobs:
  comment:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/github-script@v6
        with:
          script: |
            github.rest.issues.createComment({
              issue_number: context.issue.number,
              owner: context.repo.owner,
              repo: context.repo.repo,
              body: '👋 Thanks for reporting!'
            })

@camdencheek
Copy link
Member Author

camdencheek commented Oct 12, 2023

@jhchabran, thanks for the suggestion! Will add.

I converted this back to draft because I couldn't get the "override this check" working, and I think we want a "break glass" option. Do you have recommendations on how to handle that?

@jhchabran
Copy link
Member

@camdencheek there's AFAIK no easy way to do that, because we either allow specific actors to bypass all checks or we don't. I think given the context of thir particular check, it should be fairly rare that someone has to break glass. And if they did, they can reach us on the support channel.

So I'm in favour of adding it as is, and we could totally print out a message telling folks to do so in the GitHub action error, or if you want to take it further, it's not hard to post a comment from the action itself on the PR to really surface that. I'm fine with both.

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.

None yet

3 participants