Add "disable-empty-commits" option #81
Conversation
As discussed in 80. Note: I'm very much not a TS/JS dev, so I hope I've gotten it right ;-) Also wasn't sure how to add tests for this, but hopefully you don't mind guiding me on that ? Feel free to push to my branch.
jrfnl
force-pushed
the
feature/add-disable-empty-commits-option
branch
from
5340735 to
cb0c245
Compare
|
@Andrew-Chen-Wang Could you please approve the workflow run again ? |
|
Thanks @Andrew-Chen-Wang - looking at the current build failure - that looks to be a permission issue, which I suspect is related to my PR coming from a fork ? |
|
No worries on the failed CI; I'm just looking at the CI logs itself. Can you write a unit test that has the disable empty commit option enabled? That way we'll see whether a commit is made or not (the CI should say "Up-to-date" or whatever git says whenever there are no changes) |
Done - and both cases (should commit and should not commit) should be covered via the tests I've just added. |
cli.ts
Outdated
| @@ -92,7 +92,11 @@ if (core.getBooleanInput("preprocess")) { | |||
| } | |||
|
|
|||
| await $`git add -Av`; | |||
| await $`git commit --allow-empty -m ${core.getInput("commit_message")}`; | |||
| if (core.getBooleanInput("disable_empty_commits")) { | |||
| await $`git commit -m ${core.getInput("commit_message")}`; | |||
There was a problem hiding this comment.
Looks like this line is incorrect - the await will never receive anything when no commit is made due to no changes.
As I'm not a JS dev, I'm not sure what to do about this. Guidance welcome.
There was a problem hiding this comment.
I have a feeling this is what the original contributor, jcbhmr, ran into.
Give this a try:
| await $`git commit -m ${core.getInput("commit_message")}`; | |
| try { | |
| await $`git commit -m "Test"`; | |
| } catch (e) { | |
| if (e.exitCode === 1 && e.stderr.includes("nothing to commit")) { | |
| console.log("nothing to commit, working tree clean"); | |
| } else { | |
| throw e; // Unexpected error | |
| } | |
| } |
from chatgpt: https://chatgpt.com/share/6851dccc-4940-8010-a69a-9b22db64d4a1
something about being more defensive:
const { stdout } = await $`git status --porcelain`;
if (!stdout.trim()) {
console.log("nothing to commit, working tree clean");
} else {
await $`git commit -m "Test"`;
}
There was a problem hiding this comment.
Thanks for your original suggestion. I've applied that now 🤞🏻
As for ChatGPT - I'm pretty allergic to the garbage that puts out and the "more defensive" suggestion is nonsense in the context of this action always being run in GitHub Actions, where the git language will be English.
|
@Andrew-Chen-Wang Could you please allow CI to run again so we can see if the update I made last week made a difference ? |
|
sorry, thanks for pinging! Just ran it. Taking a look now |
Andrew-Chen-Wang
merged commit 7ee32e8
into
Andrew-Chen-Wang:master
|
Hi I've released v5.0.0 @jrfnl please take a look thank you |
|
@Andrew-Chen-Wang Uh-oh... I kind of didn't think this was ready for merge yet.... and testing v5 now, confirms that as the action fails with exit code 1 and an "uncaught exception" when there is nothing to commit. Aside from that, the I'll try and see if I can come up with a fix for the error related to this PR, but as I said before, I'm not a Node/JS dev, so haven't got a clue what I'm doing. |
As discussed in #80.
Note: I'm very much not a TS/JS dev, so I hope I've gotten it right ;-)
Also wasn't sure how to add tests for this, but hopefully you don't mind guiding me on that ? Feel free to push to my branch.