zmap
Investigate tld-update.yml workflow's PR checks
In https://github.com/zmap/zlint/commit/12dfc1846136a481fa889340d591701a3dfaa516 we landed a new github-action based automation for creating PRs with update gTLD data.
It looks like (based on this example) that the PRs the bot creates don't initiate the required workflows for the branch status checks.
The create-pull-request action we're using has details about this situation. We should investigate the workarounds to get this working properly. In the meantime it should be relatively safe to merge the bot PR's administratively skipping the CI checks. If the subsequent master builds fail we can revert the bot PR.
It also doesn't look like the branch gets deleted automatically as specified in the conf. I deleted manually.
It also doesn't look like the branch gets deleted automatically as specified in the conf. I deleted manually.
Reading the docs more it sounds like this is the intended behaviour. The config element is mildly confusing and is specific to a circumstance where the PR is closed because the bot detects that master updated such that there's no longer a diff in the PR. In this specific case it'll both close the PR and delete the branch.
We can achieve what we want here by setting the repo-wide setting to delete the HEAD branch of PRs after merge. I just enabled that now:
That also has the upside of deleting feature branches for human made PRs too.
We should investigate the workarounds to get this working properly. In the meantime it should be relatively safe to merge the bot PR's administratively skipping the CI checks. If the subsequent master builds fail we can revert the bot PR.
I looked into the possible workarounds and I think for our case there are ~two~ three options worth considering:
-
We leave things how they are now, no workflow actions for bot opened PRs. Disadvantages: This runs the risk of the bot one day proposing a commit that would break master CI without us knowing. Advantages: Better security/less complex config. We don't need a separate bot account that has write access to the repo. Update: I added a build/unit test pass to the autopull workflow to help address the risk of failures without CI on the PR.
-
We restore @tld-update-bot's access to the repo and use that acct's PAT for the workflow Disadvantages: We're back to having to manage a 2nd Github account/2FA creds and give the account write access to the repo. Advantages: The bot's PRs will have the CI workflows run on them automatically, ensuring they don't break master.
-
We don't restore @tld-updat-ebot's access, but do use it's PAT to open PRs from a fork Disadvantages: We're back to having to manage a 2nd Github account/2FA creds. Advantages: The bot's PRs will have the CI workflows run on them automatically, ensuring they don't break master.
Given that presently this automation only updates the gTLD map using go generate ./... I feel fairly confident that it will never break master. In the event it does it's pretty trivial to revert. With these things in mind I have a light preference for Option 1.
(It's also maybe worth mentioning a "hack" workaround for Option 1 - if a human closes/opens the PR pre-merge it should run the automation. However it will be annoying to have to remember to do this...)
@zakird @sleevi @dadrian @christopher-henderson Do you folks have a preference here?
Given what the bot does, I’m fine without tests. It’s pretty easy to eyeball that it’s not a breaking change. If it starts to be more complex, we should reconsider.
On Sat, Dec 12, 2020 at 8:07 AM Daniel McCarney [email protected] wrote:
We should investigate the workarounds to get this working properly. In the meantime it should be relatively safe to merge the bot PR's administratively skipping the CI checks. If the subsequent master builds fail we can revert the bot PR.
I looked into the possible workarounds https://github.com/peter-evans/create-pull-request/blob/master/docs/concepts-guidelines.md#triggering-further-workflow-runs and I think for our case there are two options worth considering:
We leave things how they are now, no workflow actions for bot opened PRs. Disadvantages: This runs the risk of the bot one day proposing a commit that would break master CI without us knowing. Advantages: Better security/less complex config. We don't need a separate bot account that has write access to the repo. 2.
We restore @tld-update-bot https://github.com/tld-update-bot's access to the repo and use that acct's PAT for the workflow Disadvantages: We're back to having to manage a 2nd Github account/2FA creds and give the account write access to the repo. Advantages: The bot's PRs will have the CI workflows run on them automatically, ensuring they don't break master.
Given that presently this automation only updates the gTLD map using go generate ./... I feel fairly confident that it will never break master. In the event it does it's pretty trivial to revert. With these things in mind I have a light preference for Option 1.
(It's also maybe worth mentioning a "hack" workaround for Option 1 - if a human closes/opens the PR pre-merge it should run the automation. However it will be annoying to have to remember to do this...)
@zakird https://github.com/zakird @sleevi https://github.com/sleevi @dadrian https://github.com/dadrian @christopher-henderson https://github.com/christopher-henderson Do you folks have a preference here?
— You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub https://github.com/zmap/zlint/issues/515#issuecomment-743777429, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABREUDRNHRN3GD3XE7UIZ3SUOIKNANCNFSM4UIIHDJQ .
It occurs to me that maybe we can sidestep most of this concern by just adding a build & unit test step to the PR workflow itself. Not quite the same as seeing the status represented in the PR but we'd have pretty good assurance that things work (and CI would run as expected on the updated master post-merge too).
It occurs to me that maybe we can sidestep most of this concern by just adding a build & unit test step to the PR workflow itself.
This seemed like a good idea either way so I opened a PR: https://github.com/zmap/zlint/pull/521 It doesn't run integration tests but I think that's reasonable in this context. Once this is merged the TLD update workflow will fail if the diff the bot creates won't pass a build/unit test phase. No PRs will be opened in this case.
I updated my earlier comment. Working through how I'm going to handle similar automation for publicsuffix/list I realized we could have the bot open PRs from a fork as a third option. This won't require write access to the repo, but we will still have to configure a PAT and maintain the account. I'm coming around to the idea that since we already have the acct maybe we should just do this :man_shrugging: