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 support for labels and existing PR check #57

Conversation

vitalyal
Copy link
Contributor

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

Copy link
Owner

@thomaseizinger thomaseizinger left a comment

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

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

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

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

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

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
@thomaseizinger
Copy link
Owner

thomaseizinger commented Jul 22, 2021

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

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

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

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

@thomaseizinger
Copy link
Owner

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

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

And here is PR from main to main #61

@thomaseizinger
Copy link
Owner

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants