Mark MR as WIP when pushing WIP commits
What does this MR do?
It marks a merge request as WIP when you push commits to it that begin with wip:
, squash!
or fixup!
. It does this by updating the MR's title to WIP: ..
, if it's not already marked as such.
Are there points in the code the reviewer needs to double check?
I have implemented work_in_progress?
on Commit
, to match MergeRequest
, however a case could be made for just inlining this in the RefreshService
, as the functionality is not used elsewhere and the functionality is kind of specific to this use case.
The tests for this rely on a wip
branch existing that has at least one commit in it that matches the WIP rules for a commit (wip:
, squash!
, fixup!
), so they will fail here.
Also, I'm sure that the test for the RefreshService
could be cleaned up a bit, I'd love some pointers on that.
Screenshots (if relevant)
Does this MR meet the acceptance criteria?
-
Changelog entry added - [-] Documentation created/updated
- [-] API support added
- Tests
-
Added for this feature/bug -
All builds are passing
-
-
Conform by the merge request performance guides -
Conform by the style guides -
Branch has no merge conflicts with master
(if it does - rebase it please) -
Squashed related commits together
What are the relevant issue numbers?
Closes #25036 (closed)
Merge request reports
Activity
@smcgivern curious to hear your thoughts :)
marked the task Changelog entry added as completed
- Resolved by Sean McGivern
- Resolved by Jurre
- Resolved by Sean McGivern
- Resolved by Sean McGivern
- Resolved by Sean McGivern
- Resolved by Sean McGivern
@jurre do you think we should automatically mark a new merge request as WIP if it contains WIP commits? Do you think we should unmark it as WIP if we had marked it as WIP based on a commit message, and there are no WIP commits now?
Semi-related: It might make sense to wrap everything in
MergeRequests::RefreshService#execute
in a transaction, because now if it fails somewhere it might create multiple notes when retrying the background job. It seemed out of scope for this MR though.That makes sense to me! I agree it's out of scope, though
assigned to @jurre
do you think we should automatically mark a new merge request as WIP if it contains WIP commits?
I have thought about it and I' not sure, you're already explicitly setting the title when you open the merge request so it could be a very conscious decision to not mark it as WIP? On the other hand, I do think it would be more consistent to do it.
An alternative could be to show a message somewhere saying something like "Hey, it looks like you opened this merge request with some WIP commits, want to mark it as WIP?" in this scenario.
Do you think we should unmark it as WIP if we had marked it as WIP based on a commit message, and there are no WIP commits now?
Yes! I actually meant to mention this in the MR description as something we should think about, but yeah, I think that would be a really nice addition.
Would you like me to work on those within this MR or do you think it's something we could add later on?
Edited by Jurreadded 1 commit
- 676f5985 - fixup! Mark MR as WIP when pushing WIP commits
added 1 commit
- 03d3a3c5 - fixup! Mark MR as WIP when pushing WIP commits
added 1 commit
- 03d3a3c5 - fixup! Mark MR as WIP when pushing WIP commits
added 1 commit
- e5a80337 - fixup! Mark MR as WIP when pushing WIP commits