Question: how do you make a PR that included WIP in a commit no longer a WIP #53
Comments
|
Thanks for opening the issue, that’s a very good question, I think we should address it in the README and the app’s description. You’ll have to update the commits for the pull request. You usually do that locally on your machine and then |
|
Got it. I'm familiar with that process. So basically have to get any
commits with WIP out of the PR. Sounds like Labels and Titles are the way
to go then for ease of use. Unless you're working on something that you're
quite sure will always be a WIP (e.g. disposable prototype) and want to
make it difficult to convert to non-WIP.
…On Thu, Mar 8, 2018 at 11:55 AM Gregor Martynus ***@***.***> wrote:
Thanks for opening the issue, that’s a very good question, I think we
should address it in the README and the app’s description.
You’ll have to update the commits for the pull request. You usually do
that locally on your machine and then git push -f to the branch of the
pull request. There are some tutorials out there, my fav is
https://egghead.io/courses/how-to-contribute-to-an-open-source-project-on-github,
the final chapters are about cleaning up the commit history of a pull
request
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#53 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AALqnKLPNujzrpjMcA6LokMnvbiInmJTks5tcWKIgaJpZM4Si_C6>
.
|
|
Yeah, it'd be nice if there was a way to disable the check on commit messages. Having to tell developers that they need to rebase their commits (some may not be comfortable with rewriting git history, etc) isn't great (especially for what is otherwise a very simple low overhead tool). |
|
Any reason not to use the label or title in that case? Seems easy to add
and easy to change.
…On Wed, Mar 14, 2018 at 12:59 Michael Warkentin ***@***.***> wrote:
Yeah, it'd be nice if there was a way to disable the check on commit
messages. Having to tell developers that they need to rebase their commits
(some may not be comfortable with rewriting git history, etc) isn't great
(especially for what is otherwise a very simple low overhead tool).
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#53 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AALqnGQCMkg1BBy2Pa4Kn7CyfR8OYztfks5teUxvgaJpZM4Si_C6>
.
|
You don’t have to, you can use GitHub’s "squash & merge" button. The WIP App setting the status to "pending" will then just serve as a reminder to not accidentally merge it with the other two options. Does that work for you? |
|
An alternative would be to remove the check in commit messages entirely, I would not make it configurable in order to keep the WIP bot as simple as it is right now |
|
@gr2m squash and merge is subject to the same branch protection configs as the other types of merges isn't it? So developers would still be blocked if we've configured it that way? I would be 👍 to removing the check from commit messages, but I know that was originally requested by other people. Maybe worth a poll or similar to see how people feel about it? |
|
Can you just not use it organizationally and instead rely on the labels and
issue titles? What's the harm in keeping it in the tool?
…On Wed, Mar 14, 2018 at 5:00 PM Michael Warkentin ***@***.***> wrote:
@gr2m <https://github.com/gr2m> squash and merge is subject to the same
branch protection configs as the other types of merges isn't it? So
developers would still be blocked if we've configured it that way?
I would be 👍 to removing the check from commit messages, but I know that
was originally requested by other people. Maybe worth a poll or similar to
see how people feel about it?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#53 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AALqnOuTkRqIX0aW6qnAd8slzByxC2isks5teYTbgaJpZM4Si_C6>
.
|
ah yes of course, sorry I am mostly an admin on the repos I maintain so I can get around that.
I’m not sure what you mean exactly? How would you tell the WIP bot to set the state to success if there are still WIP commits on the PR? |
I guess I’m saying don’t use commits to indicate WIP ever and instead using titles/labels. That way you won't have to worry about trying to unset the status via commit later. |
|
yeah I’m thinking of doing that. |
|
Typically a WIP commit should always be the last commit (e.g. synchronizing between machines), and later be |
|
I have a commit in this pull that states that it is a I'm either for disabling that check OR when you add a |
|
I can understand making a PR a work in progress when a commit message includes "work in progress" or something similar. But the WIP check should only look at the most recent commit message and, if it doesn't include the phrase, allow the PR to be merged. |
|
Another option is to add a label “work in progress” instead, then the label will make the app set the pr status to pending. This way you can remove the label anytime you want. Another wip in a commit message would set the label again, etc. would that work for you? |
|
That sounds like a much better way to me! |
|
I like the label approach quite a bit although that would require all repos to add that label in order to work properly then, right? Quite frankly, I suggest to remove the commit message parsing. Maybe GitHub should just be smarter to populate the title when creating PRs from a set of commits: if any commit includes |
|
A probot app should be able to manage labels to create the WIP label if it doesn’t exist I believe, although that might require an increase in permissions for the app. |
|
I use WIP in my own branches when I need to leave one computer and I intend to finish the work from another machine. This lets me get my work into the main repo and then pull it down into the other machine to continue working. Later, when I'm done and I create a PR, that commit is part of that branch. Eventually, when the PR is accepted, I'll typically squash and merge, so that commit will disappear, but in my case at least the issue with the bot is its use during the PR review step (before squash has occurred). So, my vote would be to either don't look at commit names for WIP, or if you must, only look at the most recent one, not all of them. |
|
I like the commit message parsing part though. If I meet the multi-desktop scenario, I would squash the commits before reviewing the pull request, because I'm not reviewing the total change, but every single commit. It would be sad if the commit message parsing is removed, without even a chance to opt-in. Logically, the WIP should not be in the final history at all. It is a status of the pull request, not a description of the commit itself. Commit message should just focus on what is done in this commit. |
|
Why does anybody care if, at some time in the past (some previous commit), something was "work in progress"? How does that somehow indicate that that work is still "in progress" now? |
|
Someone pedantic like me would care. Because that "work in progress" has nothing to do with the change itself, it's unnecessary information in the commit message. But I understand that most user just don't care about history, given all the comments above. |
|
I usually squash the commits if I want someone else to merge the PR. But it would be nice to be able to do that directly from github.com, without the need switch to the terminal. That’s a feature that would be nice beyond WIP, so if anyone wants to build an app for that, let me know, happy to help make it happen ;) If you are interested, create an issue at https://github.com/probot/ideas/issues and mention me |
|
Squash and push a new PR or force merge to the current branch/PR?
…On Fri, Jun 8, 2018 at 21:02 Gregor Martynus ***@***.***> wrote:
I usually squash the commits if I want someone else to merge the PR. But
it would be nice to be able to do that directly from github.com, without
the need switch to the terminal. That’s a feature that would be nice beyond
WIP, so if anyone wants to build an app for that, let me know, happy to
help make it happen ;) If you are interested, create an issue at
https://github.com/probot/ideas/issues and mention me
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#53 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AALqnMI_RXI_aSJQiBi3PKNIvlY597Ftks5t6x6sgaJpZM4Si_C6>
.
|
|
Current branch/PR |
Throwing my vote behind this one as well... We just got blocked on merging a PR (which consisted of a lot of individual commits) because somewhere in those WIP was mentioned. |
|
We did too. We temporarily disabled the WIP check, merged, and added it
back.
|
|
Did anyone of you work with the new checks feature & actions? I’m thinking that it might be the perfect feature for this. I’d set status to pending and add an action "ready" to the check, if the user clicks the button, the WIP app receives the webhook and sets the status to green. So far the concept :) Here is some more info on checks & action: https://developer.github.com/changes/2018-05-23-request-actions-on-checks/. I haven’t seen that in action yet (see what I did there?), did anyone of you? |
|
We did not. But I’ll check it out.
|
It's used by a handful of services - (travis, appcenter). I believe the intention is to move discreet services that perform checks on code (potentially like WIP) to use the 'Checks' functionality. |
|
@ctolkien I know some that use the checks integration. I’m looking specifically for the "actions" feature. I will give it a try and let y’all know how it goes. Another idea to "override" a pending status set by WIP could be to look for a term like "ready" in the pull request title. The benefit over labels would be that the author will be able to set it, even if they are not a contributor to the repository |
|
@gr2m any updates related to this subject? |
|
#96 looks like a great idea! Best of luck with it 👍 |
|
The idea with checks was great and I might still implement it, but the problem is the same as with labels, the author of the pull request won’t be able to override the status themselves because custom actions for check runs require write access to the repository. I’m thinking of implementing a custom override by adding this string to the pull request description:
What do y’all think? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Pending status override is here 🎉I’ve pushed it to the beta version of the WIP app and would love all your help testing it, please see my update in the 🤖📯 Updates issue. Thank you all for your patience and help 🙏 |
|
🎉 This issue has been resolved in version 3.1.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
|
you can now add All revenue from the "pro" plan will be donated to Rails Girls Summer of Code. I only added the paid plan to make the WIP a real-life GitHub App example. If you cannot pay but depend on the pro features you can add your account with an explanation to the |
|
@gr2m we're trying to use |
|
@mwarkentin Did you put the Can you share the PR URL, even if the repo is public? You can send me a twitter dm if you prefer: https://twitter.com/gr2m. I can check the logs |
Is there some way in a later commit to tell it that it's no longer a WIP?
The text was updated successfully, but these errors were encountered: