Skip to content

Added support for labels and existing PR check#57

Closed
vitalyal wants to merge 3 commits intothomaseizinger:masterfrom
vitalyal:feature/labels-and-update-pr-support
Closed

Added support for labels and existing PR check#57
vitalyal wants to merge 3 commits intothomaseizinger:masterfrom
vitalyal:feature/labels-and-update-pr-support

Conversation

@vitalyal
Copy link
Contributor

@vitalyal vitalyal commented Jul 19, 2021

Hello,
Thanks for publishing your action. We want to use it in our flow and faced a need to add another step next to it not to fail in case PR already exists.
So here is a PR not to fail and report via output if PR was created or not. Also support for labels is added.

Note: i'm not sure how to easily do integration testing for these features so no testing performed yet

thomaseizinger
thomaseizinger requested changes Jul 20, 2021
View reviewed changes
Copy link
Owner

@thomaseizinger thomaseizinger left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for submitting a PR!
I am happy to merge these features, I think they make sense to have. Esp. idempotency is a nice one.

Note: i'm not sure how to easily do integration testing for these features so no testing performed yet

You can do that pretty easily by opening a PR on your own fork and allowing actions there. The checks here are going to fail due to permissions issues but if I see them green on your PR, I am happy to merge things.

However, there are other build failures (formatting I believe) that need to be taken care of!

src/index.ts Outdated
reviewers,
});
}
try {
Copy link
Owner

@thomaseizinger thomaseizinger Jul 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have to do nested trys? I think there should be a way of having only a single one.

Copy link
Contributor Author

@vitalyal vitalyal Jul 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

src/index.ts Outdated
setOutput('html_url', htmlUrl);
setOutput('number', pullNumber.toString());
setOutput('html_url', htmlUrl);
setOutput('pr_created', 'true');
Copy link
Owner

@thomaseizinger thomaseizinger Jul 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we name this one just created? We are in the context of a PR here anyway so the naming doesn't need to repeat that.

Copy link
Contributor Author

@vitalyal vitalyal Jul 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@vitalyal vitalyal force-pushed the feature/labels-and-update-pr-support branch 2 times, most recently from 0e878a9 to 6c18c0a Compare July 20, 2021 09:30
@vitalyal vitalyal marked this pull request as ready for review July 20, 2021 09:32
@vitalyal
Copy link
Contributor Author

vitalyal commented Jul 20, 2021

Hi, build checks on my PR got green https://github.com/vitalyal/create-pull-request/pull/1/checks

@vitalyal vitalyal force-pushed the feature/labels-and-update-pr-support branch from 6c18c0a to c3c4df2 Compare July 20, 2021 09:36
@vitalyal vitalyal requested a review from thomaseizinger July 20, 2021 09:45
@thomaseizinger
Copy link
Owner

thomaseizinger commented Jul 22, 2021 •
edited
Loading

Hi, build checks on my PR got green vitalyal/create-pull-request#1 (checks)

Thank you!
I forgot that I changed the workflows in a previous PR. Can you merge the PR on your fork and link me to the resulting workflow run that is going to be triggered? I documented this here: https://github.com/thomaseizinger/create-pull-request/blob/master/CONTRIBUTING.md

@vitalyal
Copy link
Contributor Author

vitalyal commented Jul 22, 2021

Hi @thomaseizinger , i had to fix new integration tests a bit and now the only failing test case is about reviewers because they are not configured as contributors in my fork https://github.com/vitalyal/create-pull-request/actions/runs/1055169889.
As CONTRIBUTING.md states PR should be created from master, but it is not possible to update existing PR. do you want me to create another one?

@thomaseizinger
Copy link
Owner

thomaseizinger commented Jul 22, 2021

Hi @thomaseizinger , i had to fix new integration tests a bit and now the only failing test case is about reviewers because they are not configured as contributors in my fork https://github.com/vitalyal/create-pull-request/actions/runs/1055169889.
As CONTRIBUTING.md states PR should be created from master, but it is not possible to update existing PR. do you want me to create another one?

Yeah that failing test is fine :)
Creating a new PR from your master branch would be superb, thank you for sticking with this and sorry about the testing mess! (I am open to suggestions in how to improve it.)

@thomaseizinger
Copy link
Owner

thomaseizinger commented Jul 22, 2021

Can you also update the changelog? It is missing an Unreleased section at the top at the moment.

@thomaseizinger
Copy link
Owner

thomaseizinger commented Jul 22, 2021

So actually, I realized this entire action might be pointless given the existence of the gh CLI. Do you have any experience using it inside GitHub workflows?

https://cli.github.com/manual/gh_pr_create

@vitalyal
Copy link
Contributor Author

vitalyal commented Jul 23, 2021

Ok, i've moved Ureleased section to top from the bottom of the file and assume link in that section will show more changes as long as they are merged to master.
As for gh CLI pr_create i do not have any experience with it. As i understand it is supposed to be used from local repo with changes to be pushed to PR. With your action i'm able to create PR from one remote branch to another. I also have other usecases where i need to create PR from local changes and i found https://github.com/marketplace/actions/create-pull-request action to be really useful as well

@vitalyal
Copy link
Contributor Author

vitalyal commented Jul 23, 2021

And here is PR from main to main #61

@thomaseizinger
Copy link
Owner

thomaseizinger commented Jul 23, 2021

Ok, i've moved Ureleased section to top from the bottom of the file and assume link in that section will show more changes as long as they are merged to master.

Sorry, there was a misunderstanding there. The [Unreleased] part is actually just a link reference. It shouldn't be moved but instead should have a ## [Unreleased] header at the top, similar to how it is done for [1.0.0].

If you check the rendered output, this allows the heading to be clickable: https://github.com/thomaseizinger/create-pull-request/blob/master/CHANGELOG.md

And here is PR from main to main #61

I am going to close this as a result.

@vitalyal vitalyal deleted the feature/labels-and-update-pr-support branch July 23, 2021 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thomaseizinger thomaseizinger Awaiting requested review from thomaseizinger

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@vitalyal @thomaseizinger @VitaliyLitovskyiSecurrency