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

  • Please register or sign in to reply
    Loading