-
Notifications
You must be signed in to change notification settings - Fork 425
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added output #60
Added output #60
Conversation
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.
This is a really useful change, and I'd like to see it merged.
The formatting changes obscure the actual meat of the changes, and might (?) make it less likely to be merged. I imagine that you're using prettier, and this repo should be too to avoid this kinda thing. Would you mind updating your PR with the formatting changes reverted?
Good point. I've removed all the formatting changes. |
This is a useful contribution. Any chance of fixing the merge conflict and getting it merged? |
I would also like to see this merged, unfortunately I haven't heard anything from the maintainers of this repo. I already updated the formatting per @keithlayne 's suggestion, but still haven't heard anything. If a maintainer says they'd like to merge this change and need me to resolve the conflict, I'll do it. But I won't be continuously updating this PR on the off chance that someone sees it. If you are a maintainer of this repo, please let me know if you'd like to see this merged. |
Hey @danielsht86, thanks for the contribution! I took the liberty of updating your branch from main, resolving conflicts, and pushing the changes. I'm happy to see this merged—I'd like to get 1) tests, and 2) some mention of outputs added to the docs, at least the README. 👍 |
Is it possible to get this merged in? It would be very helpful 😄 I need it for the exact case pointed out by #59 (comment) On initial push to the repo, I changed api and web files but the actions cannot run because the labels aren't up to date at the time they are checked. |
Any chance of getting this merged in or is there an alternative solution? |
Does anyone have a workaround while this PR is waiting to be merged? |
I've included the changes in the default branch of my fork and rebased the major version tag, so you could use it from there. |
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.
Hello @danielsht86 ! Thank you for the PR and sorry for the late reply!
It looks good to me! I left a minor comment. Could you please also pull the changes from the main branch and resolve the conflicts?
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.
After minor updates are added it should be good to go.
I'd document new outputs as well as examples of their use in a chapter of the |
Thank you @danielsht86 for the contribution! |
In order to use this action as one step in a series of steps in a workflow (and use the changed labels as input to downstream actions), this PR outputs the added labels and all labels as output to the action.
The only changes here are the new definition of the output and piping the new labels and all labels (taken from the result of the API call) and sending them as output via
core.setOutput