Conversation
| DESTINATION_BRANCH: 'master-mirror' | ||
| SSH_PRIVATE_KEY: ${{ secrets.BOT_SSH_PRIVATE_KEY }} | ||
| with: | ||
| args: $SOURCE_REPO $SOURCE_BRANCH $DESTINATION_REPO $DESTINATION_BRANCH |
There was a problem hiding this comment.
I deleted this script because we won't need it anymore
| env: | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| - run: npx lerna bootstrap | ||
| - run: npx lerna run integration/local --stream No newline at end of file |
NickSeagull
left a comment
NickSeagull
left a comment
There was a problem hiding this comment.
Ok, this simplifies a lot, but also it makes me worry about the fact that it locks conversation without being able to add more comments. Although I see that this would be the case for when we are about to merge it.
We should unlock the PR once the tests finish, so people in the future can comment as well, even if it is closed/merged. Maybe this can be automated?
It might be a good idea to add an automated comment that says something like
"This PR was locked to run the integration tests, and will be unlocked afterwards"
|
Ideally, we should force integration tests to run before you are able to merge. This is not true right now (we didn't find a way) but just wanted to say the ideal scenario. For me, it seems weird to lock a PR to execute integration tests, it is not obvious. I agree it can be learned if it simplifies things.
|
Yes, I ran them locking the conversation in this PR
Only if you're an admin, the checks are shown along with lint and tests, and failing checks will put that nice big red warning before merging
It definitively is, but I can't imagine a better way. Ideally, there should be some kind of "before merge" hook, but we don't have that yet. I know that this is not ideal, but using comments isn't either, and using a BTW, only the repository committers will have access to merges, so we only need ourselves to adopt "the process" and make sure we pull the trigger before happily merging |
3 participants
Description
Check Runs have been working unreliably for a while now, and they were becoming harder and harder to manage, so I've reworked the corresponding Github actions to use a simpler/standard workflow. The intention of this PR is to show how this implementation works and reach an agreement with the team about the proposed workflow with the "lock" option. I locked and unlocked the PR to trigger the integration actions. Some of the tests will likely fail because this branch doesn't include the fixes from #476
Changes
integration: all. Instead of that, the integration tests are run when the PR is locked.Why I think that this change makes sense
Locking a PR disables new comments (still allow owner comments) and means that the conversation is over. When you lock a PR you can set a reason, and this reason can be "Resolved", so this makes sense from the workflow POV. We can interpret that locking a PR as "Resolved" means that is time for integration and merge. We can always "unlock" the PR to fix issues if the tests fail and lock it again when we're done to run them again. I think that this could be an elegant solution to automate the integration tests in an easier way, but I'm totally open to alternate ideas. It is very convenient that the
lockedevent is a PR event because that allows us to remove the Github check runs management from the jobs, where I suspect we introduced a bug that was making these tests misbehave.Checks
Additional info
You can find the "Lock conversation" link in the bottom of the right side panel:
That link opens a modal where you can set the reason:
Everything is looking good so far. If you remember how this was working in previous commits, clicking on an action details sent you to the wrong commit actions and other weird stuff. All of these issues seem to be resolved here:
