Skip to content
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

Merged
merged 21 commits into from Jun 29, 2023
Merged

Conversation

danielsht86
Copy link
Contributor

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.

outputs:
  new-labels:
    description: 'List of all new labels'
  all-labels:
    description: 'List of all labels'

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

@danielsht86 danielsht86 mentioned this pull request Mar 24, 2020
Copy link

@keithlayne keithlayne left a 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?

@danielsht86
Copy link
Contributor Author

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.

@bryankaplan
Copy link

This is a useful contribution. Any chance of fixing the merge conflict and getting it merged?

@danielsht86
Copy link
Contributor Author

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.

@pje pje closed this Jun 4, 2021
@pje pje deleted the branch actions:main June 4, 2021 18:48
@pje pje reopened this Jun 4, 2021
@pje pje changed the base branch from master to main June 4, 2021 19:13
@pje pje requested a review from a team as a code owner June 4, 2021 19:13
@pje
Copy link
Member

pje commented Jun 21, 2021

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.

👍

@onx2
Copy link

onx2 commented Sep 11, 2021

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.
image

@onx2
Copy link

onx2 commented Nov 5, 2021

Any chance of getting this merged in or is there an alternative solution?

@maxisam
Copy link

maxisam commented Jan 31, 2022

image

@jakul
Copy link

jakul commented Mar 24, 2022

Does anyone have a workaround while this PR is waiting to be merged?

@daczarne
Copy link

daczarne commented Dec 22, 2022

Does anyone have a workaround while this PR is waiting to be merged?

#408

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.

@caioaao caioaao mentioned this pull request Jun 13, 2023
2 tasks
Copy link
Contributor

@MaksimZhukov MaksimZhukov left a 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?

action.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@marko-zivic-93 marko-zivic-93 left a 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.

src/labeler.ts Outdated Show resolved Hide resolved
src/labeler.ts Outdated Show resolved Hide resolved
@IvanZosimov
Copy link
Contributor

I'd document new outputs as well as examples of their use in a chapter of the README.md, I think mentioning them in the action.yml alone is not enough.

@AndreiLobanovich AndreiLobanovich requested a review from a team as a code owner June 28, 2023 13:39
@MaksimZhukov MaksimZhukov mentioned this pull request Jun 28, 2023
@AndreiLobanovich AndreiLobanovich self-assigned this Jun 28, 2023
@MaksimZhukov
Copy link
Contributor

Thank you @danielsht86 for the contribution!
Thank you all for your patience!

@MaksimZhukov MaksimZhukov merged commit 0967ca8 into actions:main Jun 29, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet