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
@smcgivern I'm more than happy to also tackle automatically removing the WIP status if WIP commits are removed, let me know if you think that's a good idea for this MR. I think I have some ideas on how to check if WIP commits were removed, but I might reach out for some pointers.
Other than that I've fixed most of the things you commented on (thanks!), just unsure about what you meant with making something bold when removing the WIP status.
- Resolved by Sean McGivern
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.
That sounds good to me! We already have some help text around the WIP status, so we could maybe just amend that in this case?
@smcgivern I'm more than happy to also tackle automatically removing the WIP status if WIP commits are removed, let me know if you think that's a good idea for this MR. I think I have some ideas on how to check if WIP commits were removed, but I might reach out for some pointers.
We'd only want to do this in the case where the MR was last marked as WIP by a commit - we shouldn't remove a manually-set WIP status. But I'm not sure we have enough metadata to do this. What do you think?
mentioned in merge request !8193 (merged)
We'd only want to do this in the case where the MR was last marked as WIP by a commit - we shouldn't remove a manually-set WIP status. But I'm not sure we have enough metadata to do this. What do you think?
I think we can come pretty close: if the commits before a (force) push contain a WIP commit and the MR is marked as WIP and the commits after the push don't contain a WIP commit anymore I think we're pretty safe?
It does leave a situation open where you've pushed a WIP commit, then unmarked it as WIP and marked it as WIP again manually, and this would be undone by removing the WIP commit.
We could also check the notes to see if the WIP was set from a commit. It does kind of feel like abusing the notes, though.
added 1 commit
- 574ca9ab - fixup! Mark MR as WIP when pushing WIP commits
We could also check the notes to see if the WIP was set from a commit. It does kind of feel like abusing the notes, though.
Yeah - I know I suggested this first, this is my fault
I think that the only way we can know that the most recent setting of WIP was due to a commit is to scan the notes, but there is nothing we can do with this other than inspect the note content, which is a bad idea. Let's drop that ideaSo, I think I'll need to do two things to wrap this up:
- 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?" when pushing a new branch with wip/fixup commits
-
Add a test that ensures we show the correct commit hash in the
Marked as WIP from commit
note
Edited by Jurreadded 1 commit
- 801802bb - Show a custom WIP help message for MR's with WIP commits
@smcgivern the copy for this needs to be better, I'll think about it as well but I would love your thoughts:
@jurre I think we should put it on a separate line, so the action stays at the start of the line. Otherwise, looks reasonable to me!
Thanks, I think that's a good idea!
(4e6629e3)
added 1 commit
- 4e6629e3 - fixup! Show a custom WIP help message for MR's with WIP commits
@smcgivern technically, something like this will work:
diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index 7d542f5..d934b54 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -243,18 +243,36 @@ describe MergeRequests::RefreshService, services: true do source_branch: 'wip', target_branch: 'master', target_project: @project) + refresh_service = service.new(@project, @user) + allow(refresh_service).to receive(:execute_hooks) + allow(refresh_service).to receive(:find_new_commits) + refresh_service.instance_variable_set("@commits", [ + instance_double( + Commit, + id: 'aaaaaaa', + short_id: 'aaaaaaa', + title: 'fixup! Fix issue', + work_in_progress?: true, + to_reference: 'aaaaaa' + ), + instance_double( + Commit, + id: 'bbbbbbb', + short_id: 'bbbbbbb', + title: 'Fix issue', + work_in_progress?: false + ) + ]) commits = fixup_merge_request.commits oldrev = commits.last.id newrev = commits.first.id - refresh_service = service.new(@project, @user) - allow(refresh_service).to receive(:execute_hooks) refresh_service.execute(oldrev, newrev, 'refs/heads/wip') fixup_merge_request.reload expect(fixup_merge_request.work_in_progress?).to eq(true) - expect(fixup_merge_request.notes.last.note).to match( - /marked as a \*\*Work In Progress\*\* from #{Commit.reference_pattern}/ + expect(fixup_merge_request.notes.last.note).to eq( + "marked as a **Work In Progress** from aaaaaa" ) end
We wouldn't need the separate branch here in this case so the test could be simplified somewhat.
However, mocking out so much stuff here doesn't give me great confidence in the test. Even though we unit test this stuff, I would like to have some tests to hit all of the actual code.
We could have two separate tests of course, one that uses the instance doubles and one that tests the actual commits. I'm not a huge fan but I can't think of any other way to test this right now.
I have noticed wanting to have some sort of object that represents a collection of commits though, so that we could unit test that to make sure that it has a
first_wip_commit
method that returns the correct commit. But I don't know the codebase well enough to tell if that makes sense, and it seems like a huge change for such a small feature.Edited by JurreWe could have two separate tests of course, one that uses the instance doubles and one that tests the actual commits. I'm not a huge fan but I can't think of any other way to test this right now.
I'd be happy with more detailed tests in the style of your comment, and then a test branch in
gitlab-test
to do an 'end to end' test with a real repo, but just a single branch that we can expect a particular commit to mark the MR as WIP from.I have noticed wanting to have some sort of object that represents a collection of commits though
We don't have that for commits ATM, but we do have a
FileCollection
for diff files. I'm tempted to say YAGNI for now, if - like you said - this was the only thing we needed it for.I'd be happy with more detailed tests in the style of your comment, and then a test branch in gitlab-test to do an 'end to end' test with a real repo, but just a single branch that we can expect a particular commit to mark the MR as WIP from.
Alright, let's go for that!
We don't have that for commits ATM, but we do have a FileCollection for diff files. I'm tempted to say YAGNI for now, if - like you said - this was the only thing we needed it for.
Yeah let's not introduce more complexity than strictly needed. Just food for future thought :)
added 1 commit
- ca1357d6 - fixup! Mark MR as WIP when pushing WIP commits
@smcgivern in ca1357d6 I've added a more detailed test for referencing the correct commit, lmk what you think!
added 1359 commits
-
ca1357d6...313aa339 - 1349 commits from branch
gitlab-org:master
- 2edfb19c - Mark MR as WIP when pushing WIP commits
- 6a681926 - fixup! Mark MR as WIP when pushing WIP commits
- b7fd1f88 - fixup! Mark MR as WIP when pushing WIP commits
- 1e4812d0 - fixup! Mark MR as WIP when pushing WIP commits
- 6a4853e7 - fixup! Mark MR as WIP when pushing WIP commits
- 56cf4460 - fixup! Mark MR as WIP when pushing WIP commits
- 592cf939 - fixup! Mark MR as WIP when pushing WIP commits
- b4c0b3dc - Show a custom WIP help message for MR's with WIP commits
- 6cd99d01 - fixup! Show a custom WIP help message for MR's with WIP commits
- 28e4d250 - fixup! Mark MR as WIP when pushing WIP commits
Toggle commit list-
ca1357d6...313aa339 - 1349 commits from branch
added 66 commits
-
28e4d250...ec4fe443 - 64 commits from branch
gitlab-org:master
- b2bc68ae - Mark MR as WIP when pushing WIP commits
- 02156ab9 - Show a custom WIP help message for MR's with WIP commits
-
28e4d250...ec4fe443 - 64 commits from branch
@smcgivern I've rebased and (auto)squashed the MR, I think it should be in a pretty good state right now but happy to work on it further if you spot anything.
- Resolved by Sean McGivern
- Resolved by Sean McGivern
- Resolved by Sean McGivern
@jurre thanks, could you look at the failing tests please? Some seem unrelated to this change, but https://gitlab.com/jurre/gitlab-ce/builds/8182907 - for instance - does seem related.
@smcgivern yes, I'll fix those. We'll also need to run this against a repo with that
wip
branch in it for all the tests to pass.@jurre is that in the gitlab-test repo? If so, can you create an MR from a fork to https://gitlab.com/gitlab-org/gitlab-test/tree/wip for the changes that need to be there?
mentioned in merge request gitlab-test!16 (merged)
@smcgivern done in gitlab-test!16 (merged). There's a couple of tests failing that are not related to this WIP branch. I will fix those and look into the other things you mentioned as well.
Thanks!