Replace splices with reusable workflows

[danielrbradley]

Part of #936

• Reduce duplication in rendered workflows.
• Splices are not composable (can't call one splice from another) so don't form good building blocks.

[t0yv0]

Splices had a reason to be, avoiding breaking branch protections, I think in #687

We might want to make sure that the protections still work or just follow up here as needed.

[blampe]

This is lovely!

Splices had a reason to be, avoiding breaking branch protections, I think in #687

AFAICT the job names haven't changed so existing protections should be unaffected. We should do #693 at some point - similar to how pu/pu only checks ci-ok.

[danielrbradley]

Splices had a reason to be, avoiding breaking branch protections, I think in #687

We might want to make sure that the protections still work or just follow up here as needed.

Yup, testing for the branch protections makes sense - will test this via the xyz provider.

Reading through that thread, the discussion of the sentinel was interesting, though I think we should consider dropping the sentinel job altogether and just make our branch protections accurate for the jobs we care about.

The very precise combinations of conditionals and dependencies required to make it effective seem error-prone and non-obvious as to if there's a bug or not. The sentinel was introduced when our branch names were changing every week and so it was hard work keeping them in sync, but the sentinel was ultimately a poor attempt to avoid managing the rules. Our management of branch protections has always been pretty unreliable and gets out of sync easily. I think a bit of a redesign around this could make things simpler and more reliable.

An alternative design for the branch protections would be for the provider-ci CLI tool to have a command such as provider-ci sync-branch-protections or something similar which can be run by the repository itself to ensure it maintains the correct settings.

[danielrbradley]

The name of the step does change because of the nested worklow:

-build_sdk (dotnet)
+build_sdk / build_sdk (dotnet)

Option 1: Avoid the name change by abstracting using a composite action rather than a reusable workflow. This makes the steps slightly less nice to look at in the UI, but avoids changing the naming.

Option 2: Stick with "sentinel" checks right at the end but make them simpler to comprehend and harder to mess up. Rather than relying on the job within the workflow to be marked as complete, let's just explicitly post to our own custom check to say "all required steps passed" and only require this one check.

  sentinel:
    name: sentinel
    needs:
    - test
    - license_check
    - lint
    runs-on: ubuntu-latest
    steps:
    - uses: guibranco/github-status-action-v2@latest
      with: 
        authToken: ${{secrets.GITHUB_TOKEN}}
        context: 'Sentinel'
        description: 'All required checks passed'
        state: 'success'
        sha: ${{github.event.pull_request.head.sha || github.sha}}

This avoids the need for expressions like if: (github.event_name == 'repository_dispatch' || github.event.pull_request.head.repo.full_name == github.repository) && ! cancelled() and contains(needs.*.result, 'failure') || contains(needs.*.result, 'cancelled') || contains(needs.*.result, 'skipped'). This step can also be refactored without affecting the required checks breaking - as long as the value of context doesn't change.

[t0yv0]

I think all prior art was done in a tremendous rush. If got time to think it through and especially test on xyz provider I think it's completely reasonable to rewrite from the ground up. The key is non-disruptive rollout that does not affect provider ops.

[danielrbradley]

Ok, going to simplify the sentinel, roll it out, then come back to roll this out. Thanks for the ideating!

[mjeffryes]

I would recommend syncing with @iwahbe and @guineveresaenger about the structure of the current sentinel checks before replacing them. IIRC, they had to do some surgery to make sure skipped and cancelled jobs reported the right status, (which is why those conditionals are so complex). Eg see the code comment in #687 (comment).

[iwahbe]

I'd like to see any changes to the sentinel before they roll out. Sentinels are required because GH actions doesn't have very intuitive behavior for how it conducts required checks, and it allows race conditions with auto-merge.

[danielrbradley]

Here's the proposal PR for a better sentinel & migration plan: #944

[guineveresaenger]

I am in favor of this. I also appreciate the improved naming where each reusable workflow has its own identifiable name.

nit: Could we... structure the files to have reusable workflows in their own folder?

[danielrbradley]

nit: Could we... structure the files to have reusable workflows in their own folder?

Do they not have to be in the .github/workflows folder for GitHub to recognise them?

[danielrbradley]

I've rebased and updated this since completing the roll-out of the simplified sentinel check.

[iwahbe]

This is a very hard change to review. I'm comfortable merging if we have a test provider that has merged to master with these workflows.

[danielrbradley]

This is a very hard change to review. I'm comfortable merging if we have a test provider that has merged to master with these workflows.

@iwahbe Yeah, diffs are a bit noisy, though hoped the separate commits would help.

My plan is to wait until tomorrow as there's the sentinel changes going out tonight, then I'll roll this to a single provider first, get a green master build, then merge.

[danielrbradley]

Test release running on the xyz provider: https://github.com/pulumi/pulumi-xyz/actions/runs/9464972790