Skip to content

Added output - #60

Merged
MaksimZhukov merged 21 commits into
actions:mainfrom
danielsht86:danielsht86/add-outputs
Jun 29, 2023
Merged

MaksimZhukov merged 21 commits into
actions:mainfrom
danielsht86:danielsht86/add-outputs

Conversation

@danielsht86

Copy link
Copy Markdown
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

@keithlayne keithlayne left a comment

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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

@danielsht86

Copy link
Copy Markdown
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

pje commented Jun 21, 2021 •

Copy link
Copy Markdown
Contributor

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

onx2 commented Sep 11, 2021 •

Copy link
Copy Markdown

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

onx2 commented Nov 5, 2021

Copy link
Copy Markdown

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

@maxisam

maxisam commented Jan 31, 2022

Copy link
Copy Markdown

image

@jakul

jakul commented Mar 24, 2022

Copy link
Copy Markdown

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

@daczarne

daczarne commented Dec 22, 2022 •

Copy link
Copy Markdown

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
1 of 2 tasks

@MaksimZhukov MaksimZhukov left a comment

Copy link
Copy Markdown
Contributor

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?

Comment thread action.yml Resolved

@marko-zivic-93 marko-zivic-93 left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread src/labeler.ts Resolved
Comment thread src/labeler.ts Resolved
@IvanZosimov

Copy link
Copy Markdown
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.

@Samriel
Samriel requested a review from a team as a code owner June 28, 2023 13:39
@MaksimZhukov MaksimZhukov mentioned this pull request Jun 28, 2023
@Samriel Samriel self-assigned this Jun 28, 2023
Andrey Lobanovich added 2 commits June 29, 2023 10:56
@MaksimZhukov

Copy link
Copy Markdown
Contributor

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

@MaksimZhukov
MaksimZhukov merged commit 0967ca8 into actions:main Jun 29, 2023
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.