RSDK-10294 & RSDK-9902 - automatically merge proto PRs when tests pass #879
Conversation
| test_passing: | ||
| if: github.repository_owner == 'viamrobotics' | ||
| needs: test | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Check Results | ||
| run: | | ||
| echo Python tests: ${{ needs.test.result }} | ||
| [ "${{ needs.test.result }}" == "success" ] |
There was a problem hiding this comment.
This creates a new single test that ensures that all our tests from the test matrix have passed. This is important because we want to have tests be marked as required so that we don't auto merge a proto PR where tests have failed, but without this we need to mark all our current tests (3.8 max, 3.10 min, etc) as required. Not only is this annoying, but it's error prone because certain cases might get phased out of the matrix or new cases might get added, and we could forget to update our required list. By having a single catch-all here and marking that as required, we ensure that we're always expecting the entire matrix to pass.
This has been tested on my local branch to ensure it works.
| uses: peter-evans/create-pull-request@v7 | ||
| with: | ||
| commit-message: '[WORKFLOW] Updating protos from ${{ github.event.client_payload.repo_name }}, commit: ${{ github.event.client_payload.sha }}' | ||
| branch: 'workflow/update-protos' | ||
| delete-branch: true | ||
| title: Automated Protos Update | ||
| body: This is an auto-generated PR to update proto definitions. Check the commits to see which repos and commits are responsible for the changes | ||
| assignees: njooma | ||
| reviewers: njooma | ||
| token: ${{ secrets.GIT_ACCESS_TOKEN }} |
There was a problem hiding this comment.
Switching to peter-evans/create-pull-request. This hasn't been tested in python specifically but it has been tested insofar as we are using this same workflow and version in the typescript SDK, which is passing (the historical failures in TS were related to the token we used and have been resolved since we switched to the REPO_READ_TOKEN).
| - name: Enable Pull Request Automerge | ||
| if: steps.cpr.outputs.pull-request-operation == 'created' | ||
| uses: peter-evans/enable-pull-request-automerge@v3 | ||
| with: | ||
| token: ${{ secrets.GIT_ACCESS_TOKEN }} | ||
| pull-request-number: ${{ steps.cpr.outputs.pull-request-number }} | ||
| merge-method: squash |
There was a problem hiding this comment.
This has not been tested because I didn't want to auto-merge anything into the repo. I think this is fine; in the worst case, we fail to automerge, get am "update protos job failed" warning which immediately alerts us to it, and then we're back where we were before (merging by hand) but with error logs that can help us understand what went wrong.
| - name: Auto approve | ||
| if: steps.cpr.outputs.pull-request-operation == 'created' | ||
| run: gh pr review --approve "${{ stps.cpr.outputs.pull-request-number }}" |
There was a problem hiding this comment.
This has not been tested. I don't have any tokens in my local repo and I didn't want to try messing around with auto-approval and merging of PRs, running non-main versions of the yml file, etc. etc.
Again, I think this is fine. If it doesn't work then we're back where we were already (merging by hand) and we have more info on how to fix this.
| # if not, then this won't work. but that's not particularly bad! it just means we'll still | ||
| # have to merge by hand, but then we can add `viambot` as a code owner and then automatic | ||
| # merging will start to work. | ||
| GH_TOKEN: ${{ secrets.GIT_ACCESS_TOKEN }} |
There was a problem hiding this comment.
It's possible this token is incorrect. This token causes actions to run as viambot but it's unclear to me without actually trying to merge something whether an approval from viambot satisfies the merge requirements. I think it might not, but viambot does have override permission in this repo so perhaps I'm wrong.
If this doesn't satisfy, then it's easy to add viambot to the list of reviewers from whom a review is required. However, I'd prefer to actually see a failure before doing that and avoid making such a change unless necessary.
stuqdog
changed the title
|
Looks good, but lets see how it actually works |
2 participants
peter-evansworkflow forNote to reviewers: see inline notes for thoughts/explanations on the code and how it was tested (or why I believe leaving it untested is acceptable)
TODO:
test, rather than all the individual ones