Allow closing pull request instead of exiting with error #1065
Conversation
There was a problem hiding this comment.
Hello @HasanHajHasan,
Thank you for your contribution!
I added some comments to your PR. It would be great if you could address them. If you need help with the adjustments, let me know and I can take over.
src/github-service.ts
Outdated
| @@ -6,7 +6,7 @@ export interface IFile { | |||
| status: string | |||
| } | |||
|
|
|||
| export default class GitHubService { | |||
| export default class GithubService { | |||
There was a problem hiding this comment.
Would you mind not renaming this class?
src/main.ts
Outdated
| @@ -19,17 +19,25 @@ export async function run(): Promise<void> { | |||
| ) | |||
| } | |||
| const githubToken: string = core.getInput('githubToken', {required: true}) | |||
| const gitHubService = new GitHubService(githubToken) | |||
| const githubService = new GithubService(githubToken) | |||
There was a problem hiding this comment.
Would you mind not renaming this variable?
src/pattern-matcher.ts
Outdated
| const response = await octokit.rest.pulls.update({ | ||
| owner, | ||
| repo, | ||
| pull_number: pullRequestNumber, | ||
| state: 'closed' | ||
| }) |
There was a problem hiding this comment.
There's a GitHubService class which is responsible for interacting with GitHub. Adding this call here mixes responsibilities. I think a cleaner code would be:
if (closePR) {
await githubService.closePullRequest(owner, repo, pullRequestNumber)
} else {
core.setFailed(`There is at least one file matching the pattern ${pattern}`)
}
Additionally, the tests for this API call are missing, which is way the PR check is failing (coverage threshold isn't reached).
src/pattern-matcher.ts
Outdated
| @@ -17,7 +22,20 @@ export async function checkChangedFilesAgainstPattern( | |||
| }) | |||
|
|
|||
| if (shouldPreventFileChange) { | |||
| core.setFailed(`There is at least one file matching the pattern ${pattern}`) | |||
| const closePR: boolean = core.getInput('closePR') === 'true' | |||
There was a problem hiding this comment.
All inputs are read in main.ts. For consistency I would read this there too.
|
Hey @xalvarez, |
|
Hi @HasanHajHasan, thanks for your changes! I'm currently on vacation and I can't properly review these changes right now. I'll take care of this PR as soon as I can. |
xalvarez
force-pushed
the
feature/close-pr
branch
from
4ebfc70 to
ff960eb
Compare
xalvarez
changed the title
There was a problem hiding this comment.
Thanks for your contribution @HasanHajHasan! I'll merge these changes and prepare a release.
Adding close PR feature as an option instead of failing it