paketo-buildpacks / github-config

Common repository configuration
Apache License 2.0
8 stars 12 forks source link

Make auto-label semver action fail if there are no patchfiles #935

Closed ForestEckhardt closed 3 months ago

ForestEckhardt commented 3 months ago

The function of this task is to check if it can auto-label anything with a semver label and return the label if it can. It is run after a check to see if there are any semver labels already set and if there are not this action will run. I think that it would then follow logically that this action should fail if there are no labels to add meaning the whole check task will fail.

Checklist

robdimsdale commented 3 months ago

For posterity:

Currently the way we use this action is as follows (edited for brevity):

jobs:
  autolabel:
    steps:
    - name: Check Minimal Semver Labels
      uses: mheap/github-action-required-labels@v3
      with:
        count: 1
        labels: semver:major, semver:minor, semver:patch
        mode: exactly

    - name: Auto-label Semver
      if: ${{ failure() }}
      uses: paketo-buildpacks/github-config/actions/pull-request/auto-semver-label@main

e.g. this workflow

So the logic here is:

This successfully adds labels (where it can) if there weren't any, but in this case the job is always reported as failing. This is very noisy and misleading.

I think it's better to have a structure that looks more like the following:

jobs:
  autolabel:
    steps:
    - name: Check Minimal Semver Labels
      id: check
      uses: mheap/github-action-required-labels@v5
      with:
        count: 1
        labels: semver:major, semver:minor, semver:patch
        mode: exactly
        exit_type: success

    - name: Auto-label Semver
      if: ${{ steps.check.outputs.status == 'failure' }}
      uses: paketo-buildpacks/github-config/actions/pull-request/auto-semver-label@main

This structure has the following logic:

However, that last point in bold requires a change to the action, which is this PR

sophiewigmore commented 3 months ago

@ForestEckhardt @robdimsdale can you explain a bit more about why we want this step to noisily fail?

sophiewigmore commented 3 months ago

I get the first bit, where we don't want the semver label check to fail, but still wondering why we need the next job to fail the job if we can't add labels? Won't that happen a lot, since not every file is in the patch file list?

ForestEckhardt commented 3 months ago

Yes it will fail but only if there is not an existing semver label and it could not apply one which is the eact scenario it should fail in because that means that the PR needs to be manually labeled. If it does not fail in this scenario the PR will pass all checks without a valid smver label attached to it.

sophiewigmore commented 3 months ago

Ah yea the bit about not wanting the PR checks to pass is what I wasn't considering - thank you both for explaining!