fix: Fix GitHub pull request mergeability for multiple required workflow runs #5057
Conversation
ajax-ryzhyi-r
requested review from
chenrui333,
nitrocode and
X-Guardian
and removed request for
a team
server/events/vcs/github_client.go
Outdated
| for _, checkRun := range checkRuns { | ||
| if checkRun.Name == expectedContext { | ||
| return CheckRunPassed(checkRun) | ||
| // Iterate through checkRuns from the end, as GitHub returns them in chronological order |
There was a problem hiding this comment.
I'm not sure the order can be relied upon. Is it documented that it shall be chronological?
Alternatively, there is the checkSuite.workflowRun.runNumber that could be added to the query and used to find the latest check run for the given check.
There was a problem hiding this comment.
I'm not sure the order can be relied upon. Is it documented that it shall be chronological?
Alternatively, there is the
checkSuite.workflowRun.runNumberthat could be added to the query and used to find the latest check run for the given check.
I couldn't find any mentions of order in the documentation; this observation is based solely on empirical evidence.
Thank you for the suggestion. It looks more reliable, and I'll try to implement it 🤝
There was a problem hiding this comment.
I'm not sure the order can be relied upon. Is it documented that it shall be chronological?
Alternatively, there is the
checkSuite.workflowRun.runNumberthat could be added to the query and used to find the latest check run for the given check.
@henriklundstrom I've rewritten the fix to use checkSuite.workflowRun.runNumber for determining the latest check run. Take a look, please.
Signed-off-by: Roman Ryzhyi <[email protected]>
Signed-off-by: Roman Ryzhyi <[email protected]>
Signed-off-by: Roman Ryzhyi <[email protected]>
Signed-off-by: Roman Ryzhyi <[email protected]>
Signed-off-by: Roman Ryzhyi <[email protected]>
|
Hi @ajax-ryzhyi-r, you have some spurious file changes in this PR that aren't relevant to your change. Can you revert them please? (Adding EOF LFs t files you haven't changed.) |
Signed-off-by: Roman Ryzhyi <[email protected]>
server/events/vcs/github_client.go
Outdated
| for _, checkRun := range checkRuns { | ||
| if checkRun.Name == expectedContext { | ||
| if checkRun.CheckSuite.WorkflowRun == nil { | ||
| return CheckRunPassed(checkRun) | ||
| } else if checkRun.Name == expectedContext { | ||
| matchedCheckRuns = append(matchedCheckRuns, checkRun) | ||
| } | ||
| } |
There was a problem hiding this comment.
Name matching must always be done, otherwise the first check in the response without a workflow run would be evaluated in place of the expected check!
for _, checkRun := range checkRuns {
if checkRun.Name != expectedContext {
continue
}
if checkRun.CheckSuite.WorkflowRun == nil {
return CheckRunPassed(checkRun)
}
matchedCheckRuns = append(matchedCheckRuns, checkRun)
}
server/events/vcs/github_client.go
Outdated
| @@ -709,6 +710,24 @@ pagination: | |||
| return reviewDecision, requiredChecks, requiredWorkflows, checkRuns, statusContexts, nil | |||
| } | |||
|
|
|||
| // GetLatestCheckRun returns the checkRun with the highest runNumber, i.e., the latest checkRun whose status we need to evaluate. | |||
| func GetLatestCheckRun(checkRuns []CheckRun) (CheckRun, error) { | |||
There was a problem hiding this comment.
I think this could just return a pointer to a CheckRun or nil if none found:
| func GetLatestCheckRun(checkRuns []CheckRun) (CheckRun, error) { | |
| func GetLatestCheckRun(checkRuns []CheckRun) *CheckRun { |
Although I do think it is confusing that the function makes no distinction among the different checks, just returns whichever has the highest run number regardless the context. It would be less confusing if the function accepted the check to look for as an argument and was called GetLatestMatchingCheckRun instead. But since there is different logic to how to match expected checks and expected workflows, I think it would make more sense to just do this logic inside of the ExpectedCheckPassed and ExpectedWorkflowPassed respectively instead.
server/events/vcs/github_client.go
Outdated
| func GetLatestCheckRun(checkRuns []CheckRun) (CheckRun, error) { | ||
| latestCheckRunNumber := 0 | ||
| for _, checkRun := range checkRuns { | ||
| if int(checkRun.CheckSuite.WorkflowRun.RunNumber) > latestCheckRunNumber { |
There was a problem hiding this comment.
This would cause a panic if there is no workflow run.
server/events/vcs/github_client.go
Outdated
| } | ||
|
|
||
| for _, checkRun := range checkRuns { | ||
| if int(checkRun.CheckSuite.WorkflowRun.RunNumber) == latestCheckRunNumber { |
There was a problem hiding this comment.
This also can cause a panic.
server/events/vcs/github_client.go
Outdated
| } | ||
| } | ||
|
|
||
| for _, checkRun := range checkRuns { |
There was a problem hiding this comment.
There is no need to iterate twice, the check run with the highest run number can be returned immediately after the first loop if it is kept as a variable pointer.
Signed-off-by: Roman Ryzhyi <[email protected]>
Signed-off-by: Roman Ryzhyi <[email protected]>
what
This PR modifies the mergeability evaluation logic for GitHub pull requests by evaluating the latest workflow check run (by comparing
runNumber's) instead of the first one returned from the GitHub API.why
There is an issue described in detail in #5048 where Atlantis treats a PR as unmergeable when there are multiple required GiHub Actions workflow runs for the last commit of the branch and the first of them is failed atlantis responds with the error
Apply Failed: Pull request must be mergeable before running apply.when attempting to use theatlantis applycommand.Upon investigating this issue, I discovered that GitHub returns all
checkRunsin chronological order when Atlantis retrieves a pull request's last commit mergeability status. However, Atlantis iterates through thesecheckRunsand retrieves the firstcheckRuninstead of the last one, which represents the current status.tests
references
Fixes the issue discussed in comments of #5048