Skip to content

Commit

Permalink
feat: improve action logs to make clear why review is dismissed (#116)
Browse files Browse the repository at this point in the history
Improve logs so it's clearer why are specific reviews dismissed or
skipped.

For each review an explanation is printed out, containing commit SHAs
for which the changed files were calculated and also including a list of
changed files owned by the reviewer (directly or indirectly through team
membership):
<img width="829" alt="image"
src="https://github.com/Balvajs/dismiss-stale-reviews/assets/16807011/d60ff030-259a-4d76-a792-50e736d7e59d">

Fixes #110 and #106
  • Loading branch information
Balvajs authored Oct 8, 2023
1 parent 7895e47 commit d79929d
Show file tree
Hide file tree
Showing 5 changed files with 127 additions and 46 deletions.
74 changes: 56 additions & 18 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.

47 changes: 42 additions & 5 deletions src/calculate-reviews-to-dismiss.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,37 +92,74 @@ export const calculateReviewToDismiss = async <TReview extends Review>({
await Promise.all(
reviews.map(async review => {
const { author } = review
let isDismissed = false

debug(`Check if review of user ${author?.login} should be dismissed`)
console.log(
`Considering review from ${author?.login} and file changes between ${review.commit?.abbreviatedOid} (reviewed commit) and ${headCommit} (head commit)`,
)

if (
!author ||
// if review author is mentioned directly as an owner of changed files, dismiss their review
(author.login && changedFilesOwners.includes(`@${author.login}`))
) {
debug(
`User ${author?.login} is owner of changed files and their review should be dismissed`,
const changedFilesOwnedByReviewAuthor = filesChangedByHeadCommit
.filter(
({ owners }) =>
!!owners.find(owner => owner === `@${author?.login}`),
)
.map(({ filename }) => filename)

console.log(
`Changed files owned by ${author?.login}:\n${changedFilesOwnedByReviewAuthor.join(
'\n',
)}`,
)

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) {
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)) {
debug(
`User ${author.login} is member of ${teamOwnership} team and their review will be dismissed`,
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',
)}`,
)

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

if (isDismissed) {
console.log(`The review from ${author?.login} will be dismissed.\n`)
} else {
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`,
)
}
}),
)
}
Expand Down
6 changes: 4 additions & 2 deletions src/group-reviews-by-commit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,11 @@ export const groupReviewsByCommit = async <TReview extends Review>({
// if commit doesn't exist, make related approve ready for dismiss and continue
console.log(
'\n',
chalk.yellow`Commit '${reviewCommit}' doesn't exist in the history. It may be because it was overwritten by force push or because it's outside of checkout depth.`,
chalk.yellow(
`Commit '${reviewCommit}' doesn't exist in the history. It may be because it was overwritten by force push or because it's outside of checkout depth.`,
),
'\n',
chalk.yellow`Approval by ${review.author?.login} will be removed.`,
chalk.yellow(`Approval by ${review.author?.login} will be removed.`),
'\n',
)
reviewsWithoutHistory.push(review)
Expand Down
44 changes: 24 additions & 20 deletions src/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,20 @@ import { getInputs } from './get-inputs.ts'

const chalk = new Chalk({ level: 2 })

const logReviewsToDismiss = (
reviewsToDismiss: { author?: { login: string } | null }[],
) => {
debug(`Reviews to dismiss: ${JSON.stringify(reviewsToDismiss, null, 2)}`)

console.log(
chalk.green(
`Reviews to dismiss: ${reviewsToDismiss
.map(({ author }) => author?.login || 'unknownLogin')
.join()}`,
),
)
}

const run = async () => {
const { ghToken, ignoreFiles, noOwnerAction, forcePushAction } = getInputs()

Expand Down Expand Up @@ -43,7 +57,7 @@ const run = async () => {
debug(`Approving reviews: ${JSON.stringify(latestApprovedReviews, null, 2)}`)

if (!latestApprovedReviews.length) {
console.log(chalk.green`No reviews to dismiss!`)
console.log(chalk.green('No reviews to dismiss!'))

return
}
Expand All @@ -57,26 +71,10 @@ const run = async () => {
ignoreFiles,
})

const reviewsToDismiss = reviewsToDismissContext.filesWithoutOwner
? latestApprovedReviews
: reviewsToDismissContext.reviewsToDismiss

if (!reviewsToDismiss.length) {
console.log(chalk.green`No reviews to dismiss!`)

return
}

debug(`Reviews to dismiss: ${JSON.stringify(reviewsToDismiss, null, 2)}`)

console.log(
chalk.green`Reviews to dismiss: ${reviewsToDismiss
.map(({ author }) => author?.login || 'unknownLogin')
.join()}`,
)

// if there are some files without history let the users know and dismiss reviews calculated for dismiss
if (reviewsToDismissContext.reviewsWithoutHistory?.length) {
logReviewsToDismiss(reviewsToDismissContext.reviewsToDismiss)

console.log(
chalk.yellow(
`Files diff can't be resolved for following reviews due to force push:\n${reviewsToDismissContext.reviewsWithoutHistory
Expand Down Expand Up @@ -114,6 +112,8 @@ const run = async () => {
}
// if there are any files without owner, dismiss all reviews
else if (reviewsToDismissContext.filesWithoutOwner) {
logReviewsToDismiss(latestApprovedReviews)

console.log(
chalk.yellow(
'Files without owner:\n',
Expand Down Expand Up @@ -149,12 +149,16 @@ const run = async () => {
</details>
`.replace(/ +/g, ' '),
})
} else {
} else if (reviewsToDismissContext.reviewsToDismiss.length) {
logReviewsToDismiss(reviewsToDismissContext.reviewsToDismiss)

await dismissReviews({
octokit,
reviewsToDismiss: reviewsToDismissContext.reviewsToDismiss,
message: 'Stale reviews were dismissed based on ownership',
})
} else {
console.log(chalk.green('No reviews to dismiss!'))
}
} catch (e) {
console.error(e)
Expand Down

0 comments on commit d79929d

Please sign in to comment.