Make action compatible with private repos. #113
Conversation
|
Example run on private repo: 
|
Wow, amazing. Thanks a lot. I need some time to check it out, even to understand it properly. Stick around, don't go far, and again, thanks a lot. |
Couple of things to look out for:
|
src/main.ts
Outdated
| import parse from "parse-diff"; | ||
| import { rexify } from "./utils"; | ||
|
|
||
| async function getDiff(octokit, context) { |
There was a problem hiding this comment.
What we are using here from context is payload.repository and payload.pull request. Maybe destructure these in the argument declaration?
src/main.ts
Outdated
| core.warning(`⚠️ Not running this workflow for waived user «${senderName}»`); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
No else here? What happens if it's not an user? Maybe warn "waivedUsers" is not applicable?
There was a problem hiding this comment.
Remove the sender type check.
Added a warning now though if sender info is missing.
src/main.ts
Outdated
| @@ -28,13 +51,11 @@ async function run() { | |||
| core.warning("⚠️ Not a pull request, skipping PR body checks"); | |||
| } else { | |||
| if (bodyContains || bodyDoesNotContain) { | |||
| const PRBody = context?.payload?.pull_request?.body; | |||
There was a problem hiding this comment.
Again, the action can't continue if context does not exist; if payload does not exist, either. It's probably best to check early, and then just go with it, don't you think so?
| let additions: number = 0; | ||
| files.forEach(function (file) { | ||
| additions += file.additions; | ||
| file.chunks.forEach(function (chunk: parse.Chunk) { |
There was a problem hiding this comment.
Yeah, I removed an if condition that decreased indentation by 1 level.
| ); | ||
| } | ||
|
|
||
| core.info("Checking lines/files changed"); |
There was a problem hiding this comment.
Don't know what happened here, seems to be the same except for indentation...
There was a problem hiding this comment.
Ack, explained above.
That was one of my questions, probably something should be conveyed to the user in that case, but so far I didn't know such kind of users existed... Might be worth the while a separate PR, maybe, although what is there so far looks great.
In diff size, you mean? At any rate, maybe add a caveat in the documentation? |
|
Also, waivedUsers is not working. I have to get around to it since summer #106 |
|
Have responded to and fixed everything. @JJ Havent tested the latest set of changes, is there some way to test it out? |
|
Just the basics, I just approved running them. I don't have time ATM but I would say all looks good... Thanks! |
|
Just confirmed itsworking, please go ahead and merge. |
Make action compatible with private repos.
[x] Unit tests run and pass.