-
Notifications
You must be signed in to change notification settings - Fork 91
chore: GitHub upgrades and allow arguments #46
base: master
Are you sure you want to change the base?
Conversation
Parsed only when allow_arguments is set
@zeit/ncc is deprecated
de5ec82
to
e682ec0
Compare
The updated action is published in the marketplace if anyone wants to use it until this repo gets patched. |
@shanegenschaw would you like to merge my PR #34 in your fork? Mentioned your fork on #34 (comment) |
@shpingalet007 sure I will try to work on this asap |
PR is ready here if you wouldn't mind having a look @shpingalet007 : shanegenschaw#3 |
Okay, done @shanegenschaw! Thanks 🤝 Btw, why is this a PR instead of issue?) |
Merging contribution from shpingalet007 to allow arguments
The work from @shpingalet007 has been merged in with this PR and published as |
hasTrigger = regexTrigger.test(body); | ||
} | ||
|
||
if ((prefixOnly && !hasTrigger) || !hasTrigger) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking for prefixOnly
here is superfluous. Can be rewritten to
if (!hasTrigger) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related to PR #34. Would check this soon and apply there as this PR is duplicate. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, took a look at that @ramiro.
Finally, removing prefixOnly
everywhere as the functionality is not stable if trigger is somewhere in the middle of comment. Commit 8963ccd. The condition was changed to the suggested too as there is no need in prefixOnly
.
As this is duplicate PR, I suppose the issue must be resolved here or this PR must be closed before this review can be resolved. Anyway, the original PR is fixed. Thanks for your suggestions, and if you have more related to arguments parsing, consider publishing them in #34.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shpingalet007 can you share more details of the kind of problems the prefix_only
option introduces?
I've had cases of the official action from this repository master at the f8ab347 commit failing randomly to detect the comment keyword and I want to understand if this could be the reason.
@shanegenschaw this PR is duplicate of #34, #36, #37. If there are no reasons to keep it, the work must be proceeded there. |
Update to Node 20 and update dependencies
GitHub upgrades
Upgrades for deprecated GitHub Actions functionality:
This PR basically combines the following PRs, but adds a necessary rebuild of the
./dist/index.js
file:Resolves:
node16
instead ofnode12
#35set-output
command is deprecated and will be disabled soon #40Allow arguments
Contribution from @shpingalet007 to allow parsing arguments from the comment body.
Related PRs: