fix: correct reading of sync-labels input.#480
Conversation
|
Hello @adam-azarchs ! Thank you for the contribution! |
As my comment makes clear, I didn't want to do that because doing so would cause immediate breakage for everyone who was using the workaround of setting |
|
(granted it's still a breaking change, but only for people who were setting |
|
@adam-azarchs thank you for the explanation! |
|
Thinking more about it, I agree, since the failure mode will be the action failing loudly, as opposed to just not doing what the user wanted. I'll make that change. |
MaksimZhukov
left a comment
MaksimZhukov
left a comment
There was a problem hiding this comment.
@adam-azarchs thank you for resolving the comment, I left a few more, please take a look.
Contrary to the assumptions made in the unit tests, core.getInput always returns a string, and !!'false' is true. Also updates the unit test to reduce changes of regressions by ensuring that the mocked getInput returns a string, as it would in production. Fixes actions#112 Make sure test catches regressions.
|
Thanks for the review so far! I believe I've addressed all of the feedback at this point; please let me know if there is anything further to do (I'm not in a rush to merge this or anything, but I want to know if I can cross it off my to-do list...) |
|
Hello @adam-azarchs! Thank you! |
|
There are 23 other open PRs right now. Are any of them planned to get into the next release as well? |
|
Hello @dfandrich ! |
|
Super!
|
|
Is there any news regarding the release of this fix? The bug is quite annoying. |
|
Hello @adam-azarchs! Could you please resolve the conflicts? We would like to merge the PR |
|
Thank you all for fixing #112! |
This reverts commit 751921c.
The issue[0] for this workaround has been resolved[0], so we can set it to a proper boolean field. [0] systemd#18671 [1] actions/labeler#480
The issue[0] behind this workaround has been resolved[1], so we can set it to a proper boolean field. [0] systemd#18671 [1] actions/labeler#480
The issue[0] behind this workaround has been resolved[1], so we can set it to a proper boolean field. [0] systemd#18671 [1] actions/labeler#480
The issue[0] behind this workaround has been resolved[1], so we can set it to a proper boolean field. [0] #18671 [1] actions/labeler#480
7 participants
Description:
Correctly check the truth value of the
sync-labelsinput.Contrary to the assumptions made in the unit tests, core.getInput always returns a string, and
!!'false'is true.Related issue:
Fixes #112
Check list:
BREAKING CHANGE: previously, any non-empty string would result in labels being deleted. With this change, an empty string will cause an error, but strings representing yaml-compliant boolean values will be correctly interpreted.