Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mob start with ci skip option #354

Conversation

takaiyuk
Copy link
Contributor

@takaiyuk takaiyuk commented Jan 11, 2023

Summary

Implement CI skipping function when mob start.
Check out #353 for details.

Implementation (editted)

  • Skip CI when mob start
    • CI is skipped by committing empty with message mob start [ci-skip] [ci skip] [skip ci].
  • Remove --push-option ci.skip
    • It may by effective only for GitLab at the moment.
    • This requires us to fix test: 5810d58
  • Drop the initial empty commit when mob done --squash-wip
    • It is removed by markStartCommitForDropping().
    • It is squashed when mob done or mob done --squash.
    • It is not removed when mob done --no-squash.

@takaiyuk takaiyuk marked this pull request as ready for review January 12, 2023 00:06
@takaiyuk takaiyuk marked this pull request as draft January 12, 2023 00:26
@takaiyuk takaiyuk force-pushed the feature/mob-start-with-ci-skip branch 4 times, most recently from bfb522a to 2715740 Compare January 14, 2023 14:49
@takaiyuk takaiyuk marked this pull request as ready for review January 14, 2023 14:49
@takaiyuk takaiyuk force-pushed the feature/mob-start-with-ci-skip branch 2 times, most recently from df3bc70 to 8c4223f Compare January 15, 2023 00:43
@takaiyuk takaiyuk marked this pull request as draft January 16, 2023 00:36
@takaiyuk takaiyuk force-pushed the feature/mob-start-with-ci-skip branch 2 times, most recently from aa504d2 to 66f86a0 Compare January 17, 2023 03:22
@takaiyuk takaiyuk force-pushed the feature/mob-start-with-ci-skip branch from 66f86a0 to 9a1f8cb Compare January 17, 2023 03:25
@takaiyuk takaiyuk force-pushed the feature/mob-start-with-ci-skip branch from 9a1f8cb to f0300b8 Compare January 17, 2023 03:38
@takaiyuk takaiyuk marked this pull request as ready for review January 17, 2023 06:20
Copy link
Collaborator

@gregorriegler gregorriegler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, well done.
I think we can omit the configuration code all together, as we always want to skip the CI.
We made the same decision when we implemented the push options.
Then the change becomes much smaller.
Less code = better, less maintenance.

README.md Outdated Show resolved Hide resolved
@@ -14,6 +14,8 @@ const (
Squash = "squash"
NoSquash = "no-squash"
SquashWip = "squash-wip"

StartCISkipCommitMessage = "mob start [ci-skip] [ci skip] [skip ci]"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels weird that this constant is in the same place as the other 3 that share a concept. But I don't know how to solve that better now.

Copy link
Contributor Author

@takaiyuk takaiyuk Jan 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To define it as variable instead of constant is one of the options.

Copy link
Contributor Author

@takaiyuk takaiyuk Jan 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, introduce new type, something like SquashOption, to clarify the difference:

type SquashOption string

const (
	Squash    = SquashOption("squash")
	NoSquash  = SquashOption("no-squash")
	SquashWip = SquashOption("squash-wip")

	StartCISkipCommitMessage = "mob start [ci-skip] [ci skip] [skip ci]"
)

configuration/configuration.go Outdated Show resolved Hide resolved
@gregorriegler
Copy link
Collaborator

LGTM, can you please increase the version number to 4.2.0 in

mob/mob.go

Line 29 in 4ac1d3e

versionNumber = "4.1.2"

and add a changelog entry describing what we did?
I think then we can merge.

@takaiyuk
Copy link
Contributor Author

I did, please fix the changelog message if necessary.

Thanks for discussing and reviewing, @gregorriegler! I am so excited for my PR to be released.

@gregorriegler gregorriegler merged commit fcd1f63 into remotemobprogramming:main Jan 19, 2023
@takaiyuk takaiyuk deleted the feature/mob-start-with-ci-skip branch January 19, 2023 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants