Skip to content
Snippets Groups Projects

Mark MR as WIP when pushing WIP commits

Merged Jurre requested to merge jurre/gitlab-ce:wip-mr-from-commits into master

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)

Screen_Shot_2016-12-14_at_16.15.49

Does this MR meet the acceptance criteria?

What are the relevant issue numbers?

Closes #25036 (closed)

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Sean McGivern
  • 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 :slight_smile:

  • Sean McGivern added ~164274 label

    added ~164274 label

  • assigned to @jurre

  • Author Contributor

    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 Jurre
  • Jurre added 2 commits

    added 2 commits

    • c8d53920 - fixup! Mark MR as WIP when pushing WIP commits
    • 58c11d46 - fixup! Mark MR as WIP when pushing WIP commits

    Compare with previous version

  • Jurre added 1 commit

    added 1 commit

    • 676f5985 - fixup! Mark MR as WIP when pushing WIP commits

    Compare with previous version

  • Jurre added 1 commit

    added 1 commit

    • 03d3a3c5 - fixup! Mark MR as WIP when pushing WIP commits

    Compare with previous version

  • Jurre added 1 commit

    added 1 commit

    • 03d3a3c5 - fixup! Mark MR as WIP when pushing WIP commits

    Compare with previous version

  • Jurre added 1 commit

    added 1 commit

    • e5a80337 - fixup! Mark MR as WIP when pushing WIP commits

    Compare with previous version

  • Author Contributor

    @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.

  • Jurre marked as a Work In Progress

    marked as a Work In Progress

  • @jurre:

    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?

  • Jurre mentioned in merge request !8193 (merged)

    mentioned in merge request !8193 (merged)

  • Author Contributor

    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.

  • Jurre added 1 commit

    added 1 commit

    • 574ca9ab - fixup! Mark MR as WIP when pushing WIP commits

    Compare with previous version

  • 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 :disappointed: 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 idea :thumbsup:

  • Author Contributor

    So, 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 Jurre
  • Jurre added 1 commit

    added 1 commit

    • 801802bb - Show a custom WIP help message for MR's with WIP commits

    Compare with previous version

  • Author Contributor

    @smcgivern the copy for this needs to be better, I'll think about it as well but I would love your thoughts:

    Screen_Shot_2016-12-22_at_19.48.36

  • @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!

  • Author Contributor

    Thanks, I think that's a good idea!

    Screen_Shot_2016-12-28_at_15.08.54 (4e6629e3)

  • Jurre added 1 commit

    added 1 commit

    • 4e6629e3 - fixup! Show a custom WIP help message for MR's with WIP commits

    Compare with previous version

  • Author Contributor

    @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 Jurre
  • 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'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.

  • Author Contributor

    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 :)

  • Jurre added 1 commit

    added 1 commit

    • ca1357d6 - fixup! Mark MR as WIP when pushing WIP commits

    Compare with previous version

  • Author Contributor

    @smcgivern in ca1357d6 I've added a more detailed test for referencing the correct commit, lmk what you think!

  • Jurre added 1359 commits

    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

    Compare with previous version

  • Jurre added 66 commits

    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

    Compare with previous version

  • Author Contributor

    @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.

  • @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.

  • Author Contributor

    @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?

  • Jurre mentioned in merge request gitlab-test!16 (merged)

    mentioned in merge request gitlab-test!16 (merged)

  • Author Contributor

    @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!

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading