ROX-34007: Enhance auto-merge configurability#91
Conversation
| and (.name | test("'"${REQUIRED_CHECKS}"'")) | ||
| )) | ||
| | { | ||
| ALL_SUCCESS: (length > 0 and all(.conclusion == "SUCCESS" or .conclusion == "SKIPPED")) |
There was a problem hiding this comment.
Two corner cases:
- What if
REQUIRED_CHECKSdoesn't match to any actual checks? Then auto-merge will not happen even if all checksSUCCESS - Do we really want to auto-merge a PR which has all
REQUIRED_CHECKSchecksSKIPPED?
There was a problem hiding this comment.
- I think that is expected behavior. I will add a comment to the function. For example, if Konflux builds are required, but not triggered by the PR, they will not show up in the list.
- Previously, we relied on status roll up, which returns GREEN if all checks have succeeded or been skipped. I want to keep it like this (also for backwards compatibility). But your question brought up a bug in my implementation, I forgot about the StatusChecks (OSCI).
msugakov
left a comment
There was a problem hiding this comment.
I skipped checking get_combined_success_status this time because it scares me.
Will do it in the next go.
|
|
||
| inputs: | ||
| allowed-authors: | ||
| description: 'Authors to filter PRs for auto-merge (comma-separated)' |
There was a problem hiding this comment.
Nit: "Authors to filter PRs for auto-merge (comma-separated)" is confusing. How about "Authors (comma-separated) whose PRs will be auto-merged".
Also, please mention what happens when its value is empty. See related #91 (comment)
| label: | ||
| description: 'Label to filter PRs for auto-merge' | ||
| labels: | ||
| description: 'Labels to filter PRs for auto-merge (comma-separated `and` logic)' |
There was a problem hiding this comment.
I'd suggest something like "Only PRs having these labels will be merged. Multiple labels can be specified as comma-separated, each of them must be present on the PR."
Also, please mention what happens when an empty value is specified. See related #91 (comment)
There was a problem hiding this comment.
Done, please check the empty value clause.
| check_not_empty \ | ||
| DRY_RUN GH_TOKEN \ | ||
| REPOSITORY LIMIT LABELS ALLOWED_AUTHORS REQUIRED_CHECKS ALLOWED_BASE_BRANCHES |
There was a problem hiding this comment.
Hm. Since you have check_not_empty on all CLI arguments here, should required: false be changed to required: true in GitHub inputs definitions?
There was a problem hiding this comment.
The inputs are not required on the action level (mostly for backwards compatibility) and we provide reasonable defaults there.
In the script however, each of these inputs needs to be defined. If a workflow calling the action does not provide an input, the default from the action's inputs section will be used.
Alternatively, we could provide defaults again in the script (duplicate the defaults from the action definition) or work with named arguments (overhead).
TLDR: For backwards compatibility and simplicity, we should keep the current implementation.
| echo "[DEBUG] PR #${PR_NUMBER} skipped - base branch '${BASE_BRANCH}' not allowed" | ||
| continue |
There was a problem hiding this comment.
The default limit=50 does not seem particularly high. It may happen that at some point we would have >50 PRs that pass basic criteria (matching repo, labels, be open and mergeable, not be draft) but that are open against different branches.
The script will start, load 50 PRs and then filter them out. I mean not just this spot but also filtering by status.
Is there any way to randomize the results of gh pr list ... command so that there's at least a chance to get things going in case there are >50 non-eligible PRs?
Alternatively, gh pr list ... can load many more results, potentially all. We should not have more than a couple thousands open PRs (famous last words) total, much less will pass basic criteria.
The we reshuffle those results and process up to limit in one run.
There was a problem hiding this comment.
Counterpoint: Why may it be okay to have the current limit?
- Generally: Because we go through the list of PRs relatively quickly, so it shouldn't grow too much
- Dependabot: We don't observe issues with this number, it was fine before, so I assume it will be in the future.
- MintMaker: Because we go through the list of PRs relatively quickly, so it shouldn't grow too much. 50 may be a good compromise: (3 + 1) branches * 4 types of updates = 16 MintMaker PRs concurrently. Even if we keep release branches for EUS support, we could support 12 branches in parallel.
I think 50 is a good compromise for now, and I would not invest in optimizations with randomization or custom limit implementations. If we encounter >50 PRs, we can revisit this.
| gh pr review --repo "${REPOSITORY}" \ | ||
| --approve "${PR_NUMBER}" |
There was a problem hiding this comment.
I spotted the older command had || true at the end. Do you know why that was done?
There was a problem hiding this comment.
I assume it was done to continue the workflow if approval fails. I would actually prefer to let it fail (not silently), because only if it breaks, engineers will investigate and implement improvements if necessary.
| echo "[DEBUG] ✓ PR #${PR_NUMBER} - auto-merge enabled" | ||
| fi | ||
|
|
||
| # Approve only PRs by allowed authors |
There was a problem hiding this comment.
I know you did not change that logic as compared to the original implementation, but I wonder what is the scenario when we want to enable auto-merge but don't want to approve?
There was a problem hiding this comment.
You can set auto-merge label on any PR (your personal ones as well). Your PR will be checked and auto-merge enabled (although not approved) by this workflow. Scenario:
- You submit PR
- Auto-retesters do their magic until CI is green
- Only then, auto-merge workflow enables auto-merge
- Someone approves
- PR gets auto-merged
That may be useful for handing off responsibility after submitting the PR (and knowing that CI will be green eventually).
This is done in preparation for auto-merging MintMaker/Renovate updates in
stackrox/stackrox, while keeping control over the required checks outside of GH branch protection rules.Validation
Local
Default configuration
Intended use for MintMaker
(I want to update the Renovate config such that it sets
mintmakerandauto-mergelabel on each update, but we're not there yet, hence testing withkonflux-build)GHA