Skip to content

Commit

Permalink
fix: handle edge case with same review sha after force-push (#122)
Browse files Browse the repository at this point in the history
When the force-push doesn't rewrite the whole history, then it may
happen that the review SHA and head SHA are the same. This means that
the changed files can't be resolved, like when the force-push rewrites
the whole history, but the action doesn't recognize that it's
"force-push" scenario and doesn't remove any review.

Simply comparing the review SHA and head SHA can be used to catch this
scenario and then treat it as a force-push.
  • Loading branch information
Balvajs authored Oct 11, 2023
1 parent d79929d commit 733cfd2
Show file tree
Hide file tree
Showing 3 changed files with 58 additions and 47 deletions.
47 changes: 27 additions & 20 deletions dist/main.cjs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion dist/main.cjs.map

Large diffs are not rendered by default.

56 changes: 30 additions & 26 deletions src/calculate-reviews-to-dismiss.ts
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,14 @@ export const calculateReviewToDismiss = async <TReview extends Review>({
`Considering review from ${author?.login} and file changes between ${review.commit?.abbreviatedOid} (reviewed commit) and ${headCommit} (head commit)`,
)

if (
if (review.commit?.abbreviatedOid === headCommit) {
console.log(
'The review commit sha is the same as head commit sha and changed files can’t be resolved. This is caused by force-push.',
)
isDismissed = true
reviewsWithoutHistory.push(review)
reviewsToDismiss.push(review)
} else if (
!author ||
// if review author is mentioned directly as an owner of changed files, dismiss their review
(author.login && changedFilesOwners.includes(`@${author.login}`))
Expand All @@ -118,38 +125,35 @@ export const calculateReviewToDismiss = async <TReview extends Review>({

reviewsToDismiss.push(review)
isDismissed = true

return
}

// if the files are not owned by teams we can exit early, the user is already checked
if (!changedFilesTeamOwners.length) {
else if (!changedFilesTeamOwners.length) {
console.log(
`Review author ${author?.login} doesn't own any of changed files, nor is member of any team owning changed files.\nThe review from ${author?.login} won't be dismissed.\n`,
)

return
}

for (const teamOwnership of changedFilesTeamOwners) {
if (teamMembers[teamOwnership]?.includes(author.login)) {
const changedFilesOwnedByAuthorsTeam = filesChangedByHeadCommit
.filter(
({ owners }) =>
!!owners.find(owner => owner === `@${teamOwnership}`),
} else {
for (const teamOwnership of changedFilesTeamOwners) {
if (teamMembers[teamOwnership]?.includes(author.login)) {
const changedFilesOwnedByAuthorsTeam = filesChangedByHeadCommit
.filter(
({ owners }) =>
!!owners.find(owner => owner === `@${teamOwnership}`),
)
.map(({ filename }) => filename)

console.log(
`Review author ${author?.login} is member of ${teamOwnership} team, which owns following changed files:\n${changedFilesOwnedByAuthorsTeam.join(
'\n',
)}`,
)
.map(({ filename }) => filename)

console.log(
`Review author ${author?.login} is member of ${teamOwnership} team, which owns following changed files:\n${changedFilesOwnedByAuthorsTeam.join(
'\n',
)}`,
)

reviewsToDismiss.push(review)
isDismissed = true
} else {
debug(`User ${author.login} is not member of ${teamOwnership} team`)
reviewsToDismiss.push(review)
isDismissed = true
} else {
debug(
`User ${author.login} is not member of ${teamOwnership} team`,
)
}
}
}

Expand Down

0 comments on commit 733cfd2

Please sign in to comment.