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

Update workflows to support fork PRs #3544

Merged
merged 12 commits into from
Sep 29, 2023
Merged

Conversation

jprochazk
Copy link
Member

@jprochazk jprochazk commented Sep 29, 2023

What

Part of #1994

This duplicates certain workflows and sanitizes them to not use secrets. Anything that uses secrets must run on pull_request_target.

A similar attempt was made in #1991

Checklist

@jprochazk jprochazk added the 🧑‍💻 dev experience developer experience (excluding CI) label Sep 29, 2023
@jprochazk jprochazk marked this pull request as ready for review September 29, 2023 14:18
@jprochazk
Copy link
Member Author

This introduces another bundle of joy for anyone working on our CI:

  • Every check must separately be added to the contrib workflows, introducing an awful case of duplication
  • Every workflow that runs on pull_request must be sanitized properly, unless it uses the if repo_owner == rerun-io check (which we have to think about now)
  • Every contrib workflow must be confirmed to work by making a contribution from a different account, and for that to even be possible, it must be merged into main first

can't test this on the PR that introduces it...
@jprochazk jprochazk requested a review from jleibs September 29, 2023 14:26
Copy link
Member

@jleibs jleibs left a comment

Choose a reason for hiding this comment

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

The overall concept seems sound. A few questions regarding possible refactors, but we can also punt those until we have more time if they are too much work.

checks:
name: Checks
if: github.event.pull_request.head.repo.owner.login == 'rerun-io'
Copy link
Member

Choose a reason for hiding this comment

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

These have to be on a per-job basis rather than just skipping the whole workflow?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think so, or at least I couldn't find a better way to do this. We need to know if the PR comes from within the rerun-io organization, and there doesn't seem to be a built-in way to do this on pull_request events.

Comment on lines +10 to +16
checks:
name: "Checks"
if: github.event.pull_request.head.repo.owner.login != 'rerun-io'
uses: ./.github/workflows/contrib/checks.yml
with:
CONCURRENCY: pr-${{ github.event.pull_request.number }}
PR_NUMBER: ${{ github.event.pull_request.number }}
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't really matter, but I feel like it would be more obvious to have these in on_pull_request following the non-contrib sections.

Copy link
Member Author

@jprochazk jprochazk Sep 29, 2023

Choose a reason for hiding this comment

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

I wanted the name of the workflow to be different from the usual Pull-Request, because GH generates the full name of the job by concatenating the name with the parent workflow

Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit worried about maintainabillity here.

How much worse would it be to add a workflow_call parameter such as:

      CONTRIB:
        type: boolean
        required: false
        default: true

And then parameterizing the job instead of forking.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried it, and found it to be significantly worse. These if checks get lost in the mass of yaml. I think the duplication makes this slightly more maintainable.

@jprochazk
Copy link
Member Author

Merging so I can test an external contribution

@jprochazk jprochazk merged commit 678da91 into main Sep 29, 2023
10 of 17 checks passed
@jprochazk jprochazk deleted the jan/external-contrib-workflows branch September 29, 2023 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧑‍💻 dev experience developer experience (excluding CI) include in changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants