mob start with ci skip option

[takaiyuk]

Context

329 made --push-option=ci.skip available, which allows CI to be skipped in some use cases (perhaps only GitLab).

However, CI is not skipped when mob start in my organization, using CircleCI in GitHub. This is probably because --push-option=ci.skip makes no sense except for GitLab.

Proposal

• Introduce an opt-in option for CI skipping when mob start.
  • No change to default behavior due to opt-in option.
• Execute an empty commit with ci-skip message (e.g. mob start [ci-skip] [ci skip] [skip ci]) before push for skipping CI.
  • Commits are squashed when mob done, so there is no problem with empty commits.

Implementation Example

• Execution command

mob start --ci-skip

or

export MOB_START_WITH_CI_SKIP=true && mob start

• Output of command execution

> starting new session from origin/BRANCH-NAME
  git checkout -B mob/BRANCH-NAME origin/BRANCH-NAME
  git commit --allow-empty -m mob start [ci-skip] [ci skip] [skip ci]
  git push --push-option ci.skip --no-verify --set-upstream origin mob/BRANCH-NAME

---

Edit

• Make CI skip option be true as default.
• Initial empty commit with ci-skip message is removed when mob done --squash-wip.
  • not removed when mob done --no-squash
• --push-option=ci.skip to be deleted.

[gregorriegler]

Hi @takaiyuk, Thank you a lot for your contribution!

So if I understand you correctly, the git push --push-option ci.skip does not help with skipping the CI in your environment. However, if you did a git commit --allow-empty -m mob start [ci-skip] [ci skip] [skip ci] before, it would? Is that so? Can you verify that for me?

In that case, I would propose that we skip the configuration property all together, and make the empty commit the default behaviour. As we always would want to skip the CI anyways. There would be no value in turning it off.

Another question that would be interesting is: If the empty commit would solve the problem of skipping the CI, would there still be a reason to keep the --push-option ci.skip, or would this become obsolete?

[takaiyuk]

Hi @gregorriegler, thank you for your quick response.

So if I understand you correctly, the git push --push-option ci.skip does not help with skipping the CI in your environment. However, if you did a git commit --allow-empty -m mob start [ci-skip] [ci skip] [skip ci] before, it would? Is that so? Can you verify that for me?

Yes, you understand correctly. It is hard to show you the verification but it helped us to skip CI because it’s similar to mob next commit message (mob next [ci-skip] [ci skip] [skip ci]), which skips CI for sure as you know. (However, I will try to take a screenshot of the situation and share it with you.)

In that case, I would propose that we skip the configuration property all together, and make the empty commit the default behaviour. As we always would want to skip the CI anyways. There would be no value in turning it off.

As you pointed out, I also think that it’s better to skip CI always than leaving the default behaviour as before.

Another question that would be interesting is: If the empty commit would solve the problem of skipping the CI, would there still be a reason to keep the --push-option ci.skip, or would this become obsolete?

I do not think --push-option is needed anymore. At least at the moment, whether this option properly works probably depends on the environment. ref. https://docs.gitlab.com/ee/ci/pipelines/#skip-a-pipeline

[takaiyuk]

However, I will try to take a screenshot of the situation and share it with you.

If my latest commit is Commit as usual and executemob start, then CI is triggered on the new mob branch as expected.

If you do a git commit --allow-empty -m mob start [ci-skip] [ci skip] [skip ci] before push, then CI is not triggered as when you run mob next.

[simonharrer]

We should be clear what the consequences of such an empty WIP commit are.

Making this default makes using the mob done --merge option basically unusable.

[takaiyuk]

I cannot find out --merge option for mob done, but it is worth for us to check out what happens when mob done including the empty commit.

• mob done squashes commit messages, including the empty commit.
• mob done --no-squash leaves the empty commit.
• mob done --squash squashes commit messages, including the empty commit.
• mob done --squash-wip leaves the empty commit.

Some mob done options make the empty commit alive, so we need to consider how to deal with them. One option: always drop empty commits in the same way as only wip commits are squashed by markPostWipCommitsForSquashing() when mob done --squash-wip

[gregorriegler]

Hi @takaiyuk, I think @simonharrer is talking about mob done --squash-wip.

The reason why mob done --squash-wip leaves the empty commit is that it is not programmed to replace a commit message named mob start [ci-skip] [ci skip] [skip ci]. It would however squash it if it was mob next [ci-skip] [ci skip] [skip ci] (subtle difference, start vs next)

We first need a failing test that uses --squash-wip and fails because the empty commit is not squashed whereas it expects all wip commits to be squashed including the empty one. Then we need to adapt configuration.go. This function IsWipCommitMessage should also return true if the commit was named mob start [ci-skip] [ci skip] [skip ci] https://github.com/remotemobprogramming/mob/blob/480227b04fa33684f849bbb2e85b85053b9693ca/configuration/configuration.go#L64

This should make the failing Test pass and solve our issue.

[takaiyuk]

Hi @gregorriegler, thanks for the comment in detail.

As you explained, mob start [ci-skip] [ci skip] [skip ci] should be squashed, but it is the first commit for the wip branch (since it is committed when mob start) and you cannot squash due to without a previous commit. So, I think that the commit have to be dropped instead of squashed, which requires us to implement a function that drops the initial commit in the similar way that squashWip() does.

Another question: Should we drop the commit when mob done --no-squash? If so, we have to drop it whenever mob done.

[gregorriegler]

As you explained, mob start [ci-skip] [ci skip] [skip ci] should be squashed, but it is the first commit for the wip branch (since it is committed when mob start) and you cannot squash due to without a previous commit. So, I think that the commit have to be dropped instead of squashed, which requires us to implement a function that drops the initial commit in the similar way that squashWip() does.

Good point. You're right, we would have to drop it. And I don't think it's that complicated. There's this function squashWipGitSequenceEditor. It takes the content of an interactive rebase and marks commits for squash that need squashing: https://github.com/remotemobprogramming/mob/blob/4ac1d3e059ab603bf2464e51934da516e493bb8c/squash_wip.go#L98

func squashWipGitSequenceEditor(fileName string, configuration config.Configuration) {
    replaceFileContents(fileName, func(input string) string {
        return markPostWipCommitsForSquashing(input, configuration)
    })
}

It could also mark the first commit for dropping.

It uses this function markPostWipCommitsForSquashing which, as the name suggests, marks all commits for squashing that follow a wip commit. https://github.com/remotemobprogramming/mob/blob/4ac1d3e059ab603bf2464e51934da516e493bb8c/squash_wip.go#L143 We could write another function similar to it. Maybe call it markStartCommitForDropping, and it would mark the mob start commit for dropping. Then squashWipGitSequenceEditor would have to chain both functions: markPostWipCommitsForSquashing and markStartCommitForDropping, and we're good. There are lot's of unittests in this regard that will help get feedback. See https://github.com/remotemobprogramming/mob/blob/4ac1d3e059ab603bf2464e51934da516e493bb8c/squash_wip_test.go#L196

Should we drop the commit when mob done --no-squash? If so, we have to drop it whenever mob done.

No, I don't think the --no-squash option should do any of this. However I think we would have to increase the minor version, since the resulting commits change.

[takaiyuk]

I got it with your step-by-step explanation. I will call markStartCommitForDropping() in squashWipGitSequenceEditor() for the return value of markPostWipCommitsForSquashing() and write test in the same way as TestMarkSquashWip_manyWipCommitsFollowedByManualCommit().

OK, I will not drop the initial commit when --no-squash option.

[takaiyuk]

PR is ready: #354

[takaiyuk]

PR was merged and released as v4.2.0.