Comments (49)
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).
from app.
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
from app.
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.
from app.
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?
from app.
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 WIP
the suggestion for the PR title should probably include WIP
as well. For single commit PRs it would already work, which probably addresses the 90% case anyway...
from app.
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?
from app.
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 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?
from app.
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.
from app.
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:
@wip ready for review
What do y’all think?
from app.
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.
from app.
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?
from app.
from app.
@gr2m any updates related to this subject?
Also have you decided if you're going to remove the commit check message or at least only check for the most recent commit?
from app.
It should remove it automatically within a few seconds of the title changing; it could be that GitHub webhooks(?) is being slow to push changes, so the WIP bot doesn't know to action it yet.
from app.
@steveoh Thanks! I see an network error in the logs
Sep 12 13:51:17 event request to https://api.github.com/repos/agrc/gis.utah.gov/commits/f1dcf4b7e4175bcd4f8cb81a0ccff359939b15d9/status failed, reason: getaddrinfo ENOTFOUND api.github.com api.github.com:443
I see plenty of these lately, I’ll check in with GitHub’s API team and probably implement a retry
from app.
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
from app.
from app.
from app.
@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?
from app.
from app.
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?
ah yes of course, sorry I am mostly an admin on the repos I maintain so I can get around that.
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?
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?
from app.
yeah I’m thinking of doing that.
from app.
Typically a WIP commit should always be the last commit (e.g. synchronizing between machines), and later be --amend
ed. Having WIP commits merged to master seems wrong to me. Commits should be atomic, i.e. any commit should work on its own.
from app.
I have a commit in this pull that states that it is a work in progress
design (commit is atomic though). Not so great that this makes the thing not mergable (without admin rights)
I'm either for disabling that check OR when you add a wip-ignore
label (or similar) to the pull it will make it mergable.
from app.
That sounds like a much better way to me!
from 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.
from app.
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.
from app.
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.
from app.
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
from app.
from app.
Current branch/PR
from app.
An alternative would be to remove the check in commit messages entirely
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.
from app.
from app.
I haven’t seen that in action yet (see what I did there?), did anyone of you?
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.
from app.
@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
from app.
you’ll be able to configure terms/locations, see #96. Follow updates at #89. I’m also looking into using checks to manually override a pending status
from app.
#96 looks like a great idea! Best of luck with it 👍
from app.
I notice that even after updating a PR's title to remove WIP etc, that the status does not change. I don't think the bot is notified when the title is changed. It is only checked when commits happen, is that correct?
from app.
I'm seeing one take longer than 31 minutes. Am I being too impatient?
from app.
I just updated this already-WIP PR of mine to not be WIP and back again, and the GitHub statuses on the PR updated as expected: martincostello/api#88
from app.
I don't think the bot is notified when the title is changed
It is. Do you have "WIP" in one of the labels or commit messages?
from app.
only in the PR title. not a label or commit message.
from app.
@steveoh can you share the URL to the PR? Even if it’s private, it will help me to find it in the logs. You can also send me a twitter dm if you prefer not to share it publically: https://twitter.com/gr2m
from app.
from app.
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 🙏
from app.
🎉 This issue has been resolved in version 3.1.0 🎉
The release is available on GitHub release
Your semantic-release bot 📦🚀
from app.
you can now add @wip ready to review
to override a "pending" status, this requires the Pro plan: https://github.com/marketplace/wip.
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 pro-plan-for-free.js
file.
from app.
@gr2m we're trying to use @wip ready for review
but it doesn't seem to be doing anything - any pointers / debugging? We're on the enterprise plan afaik.
from app.
@mwarkentin Did you put the @wip ready for review
into the pull request body or in a comment? It has to be in the pull request body right now
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
from app.
Related Issues (20)
- An in-range update of semantic-release is breaking the build 🚨 HOT 2
- How to address the issue of your GitHub App being down(just in case) HOT 2
- WIP blocks merging if commit message contains "wip" HOT 6
- Label matcher stopped working HOT 8
- Handle suspended installations
- Label matcher stopped working HOT 19
- M
- WIP Server seems to be hanging HOT 5
- Add support for the Draft prefix out-of-the-box HOT 1
- Title HOT 1
- wip/alp
- github/wip.yml is being ignored HOT 11
- Add support for merge_queue HOT 2
- Add support for :construction: `:construction:` (text) HOT 1
- The user data can't be public HOT 3
- Changing Title after Build kicks off makes WIP stuck HOT 2
- New to GitHub App HOT 1
- Disable body parsing using code instead of relying on `NODEJS_HELPERS` environment variable
- WIP Expected — Waiting for status to be reported HOT 17
- Trying to add changes HOT 4
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from app.