Added repository configuration to only push tags to remote repositories #81

erichsend · 2015-05-18T20:55:41Z

This relates to #66

In a detached head state that can occur on some CI systems, the release task will fail when it attempts to push local commits to the remote repository. My attempt to fix was to add an optional configuration parameter to signal that only local tags, and not local commits, should be pushed to the remote repository.

Output of my jenkins job showing a successful push from a detached head state:
https://gist.github.com/erichsend/4b9327c58dec98e22ae2

…ush tags

coveralls · 2015-05-18T21:00:18Z

Coverage decreased (-0.1%) to 63.99% when pulling c20848e on erichsend:conditional-tag-only-push into a5de4d7 on allegro:master.

szpak · 2015-05-18T21:34:03Z

src/main/groovy/pl/allegro/tech/build/axion/release/infrastructure/git/GitRepository.groovy

@@ -83,12 +87,16 @@ class GitRepository implements ScmRepository {
    }

    private void callPush(String remoteName, boolean all) {
-        repository.push(remote: remoteName, all: all)
+        if(pushTagsOnly == false) {


!pushTagsOnly?

szpak · 2015-05-18T21:35:40Z

It would be probably good to have a simple test case for the new feature.

erichsend · 2015-05-18T22:03:31Z

@szpak I agree. I am working on a test.

adamdubiel · 2015-05-18T22:15:12Z

Waiting for the test before merge :) Could you also update the documentation? At least the overview section (in docs/).

erichsend · 2015-05-18T22:17:53Z

@adamdubiel Sure thing. I just wanted to be sure this approach was acceptable before changing docs.

…ository

vbuell · 2015-05-18T22:23:22Z

Adam, as David mentioned instead of introducing a new option we can just skip pushing if no 'releaseCommit' option specified.

coveralls · 2015-05-18T22:25:14Z

Coverage increased (+0.08%) to 64.16% when pulling 1285c3e on erichsend:conditional-tag-only-push into a5de4d7 on allegro:master.

adamdubiel · 2015-05-18T22:25:45Z

src/main/groovy/pl/allegro/tech/build/axion/release/domain/scm/ScmInitializationOptions.groovy

    private static final String ATTACH_REMOTE_PROPERTY = 'release.attachRemote'

    private static final String FETCH_TAGS_PROPERTY = 'release.fetchTags'

-    ScmInitializationOptions(String remote, boolean fetchTags, boolean attachRemote, String remoteUrl) {
+    ScmInitializationOptions(String remote, boolean fetchTags, boolean attachRemote, String remoteUrl, boolean pushTagsOnly) {


i wonder - this option might be set from cmd line as well (via -Prelease.pusthTagsOnly), so there would be no need to leak CI settings into buildscript

I agree. I wasn't sure exactly where to pipe it in, I just needed to demonstrate the functionality. I was hoping you would have the better vision for how to apply it.

adamdubiel · 2015-05-18T22:31:01Z

@vbuell - right, but then i think having manual override via cmdline for CI could be useful. The pushTagsOnly option needs to be added to Repository anyways, it's just question of how to set it's value. And then, it is not so easy to see if someone performs any commit in pre-release action, since she might use predefined action of commit type and not set releaseCommit option at all.

coveralls · 2015-05-18T22:35:13Z

Coverage increased (+0.08%) to 64.16% when pulling 6e14a1d on erichsend:conditional-tag-only-push into a5de4d7 on allegro:master.

adamdubiel · 2015-05-18T22:35:58Z

@erichsend wrapping it all up:

i should be able to set pushTagsOnly from command line
command line setting should take precedence over buildscript setting
in general i think this flag should be evaluated as late as possible (meaning passed repository#push), allowing people to override it in runtime, but this would call for some refactoring and i consider it outside the scope of this pr

adamdubiel · 2015-05-18T22:38:50Z

src/test/groovy/pl/allegro/tech/build/axion/release/infrastructure/git/GitRepositoryTest.groovy

+    def "should not push commits if the pushTagsOnly flag is set to true"() {
+        given:
+        initializationOptions = ScmInitializationOptions.fromProject(project, 'origin', true)
+        repository = new GitRepository(project.file('./repo'), identity, initializationOptions)


hm, this creates nested repository, i would be careful with that - maybe just create new Project object? (this is a bit of hack for creating temp file, but it also makes the domain look nicer :P)

… own projects and repositories

coveralls · 2015-05-19T03:24:53Z

Coverage increased (+0.08%) to 64.16% when pulling 8f9dc4d on erichsend:conditional-tag-only-push into a5de4d7 on allegro:master.

erichsend · 2015-05-20T16:31:24Z

@adamdubiel
Regarding the command line setting, should it be similar to dryRun, that is, if it is present then it is true? Or should it be able to be set as true or false from the command line property?

I currently have it set to behave like the dryRun flag:

     boolean pushTagsOnly = (project.hasProperty(REPOSITORY_PUSH_TAGS_ONLY_FLAG) ?
                true : config.pushTagsOnly)

…pushTagsOnly

coveralls · 2015-05-21T19:28:50Z

Coverage increased (+0.14%) to 64.22% when pulling 87dcf63 on erichsend:conditional-tag-only-push into a5de4d7 on allegro:master.

…pushTagsOnly configuration and flag

erichsend · 2015-05-21T19:32:28Z

@adamdubiel
I believe this is ready. Let me know if you would like me to change the command line option to accept a true/false value instead of acting as an ON flag, or any other suggestions you have.

adamdubiel · 2015-05-24T13:39:51Z

@erichsend - sorry for delay, i been on short vacation, i will take a look at this today or tomorrow

adamdubiel · 2015-05-24T20:04:33Z

...ain/groovy/pl/allegro/tech/build/axion/release/infrastructure/di/ScmRepositoryFactory.groovy

@@ -14,14 +14,19 @@ class ScmRepositoryFactory {

    private static final String GIT = 'git'

+    private static final String REPOSITORY_PUSH_TAGS_ONLY_FLAG = "repository.pushTagsOnly";


could you change the name to release.pushTagsOnly? Name clash is very unlikely for such a long flag, but i would like to keep everything in release namespace

adamdubiel · 2015-05-24T20:07:42Z

There is just one "big" remark about naming. Other than that, i feel that there is room for improvement, but it's not critical and would involve bigger refactor. I will take care of it after merge. Please change the variable name and we are set.

…release.pushTagsOnly

…constructor

coveralls · 2015-05-26T18:03:14Z

Coverage increased (+0.14%) to 64.29% when pulling 0b5228d on erichsend:conditional-tag-only-push into 1cb01ca on allegro:master.

erichsend · 2015-05-26T18:10:39Z

I agree this change is more of a work around than a good long term solution. Your suggestion of removing the flag as a property of the repository and instead adding it as an argument to the push() method certainly seems desirable. If I can find the time in the next week or two I could attempt that assuming you do not get to it first.

Another thing I would bring up is whether pushTagsOnly should be the default behavior. Maybe it is because my team has a policy to only do releases from the CI system, but I was actually surprised that the plugin was attempting to push local commits, as it seems like any commit important enough to be a release should first be published and vetted (by the CI system and peer review) before the release tag is applied.

As I said though, my experience would be quite different from someone who primarily did all of their release maintenance from their laptop.

erichsend · 2015-05-26T18:12:38Z

Regarding the naming, should the configuration item (not the flag) remain as a property in RepositoryConfig? I'm just working on the docs now and still have this example:

scm {
    repository {
        pushTagsOnly=true
    }
}

…gsOnly CLI flag

coveralls · 2015-05-26T18:32:19Z

Coverage increased (+0.14%) to 64.29% when pulling fa21a1f on erichsend:conditional-tag-only-push into 1cb01ca on allegro:master.

adamdubiel · 2015-05-26T21:41:58Z

Naming: yest, this should be part of repository configuration, but on global level (gradle flags) it should be in release.* namespace.

As to the default behavior: we also release only from CI, but mind, that when using pre-release hooks there might be some local commits that i.e. change version number in Readme or some docs, that should be pushed as well. I would rather keep the new option optional instead of default not to break it :)

Added repository configuration to only push tags to remote repositories

adamdubiel · 2015-05-26T21:44:19Z

Merged, thanks for this contribution :)

erichsend · 2015-05-27T01:08:39Z

Thanks for pulling this in, Adam, and thanks for the great plugin.

[conditional-tag-only-push]: Added repository configuration to only p…

c20848e

…ush tags

szpak reviewed May 18, 2015
View reviewed changes

[conditional-tag-only-push]: Adding test for pushTagsOnly flag on rep…

1285c3e

…ository

[conditional-tag-only-push]: Changing pushTagsOnly check to concise form

6e14a1d

adamdubiel reviewed May 18, 2015
View reviewed changes

[conditional-tag-only-push]: Refactored tag-only push test to use its…

8f9dc4d

… own projects and repositories

erichsend added 2 commits May 21, 2015 12:13

[conditional-tag-only-push]: Added command line property flag to set …

8879916

…pushTagsOnly

[conditional-tag-only-push]: Removing unused import

87dcf63

[conditional-tag-only-push]: Updating docs to explain the repository.…

9ea810a

…pushTagsOnly configuration and flag

adamdubiel reviewed May 24, 2015
View reviewed changes

[conditional-tag-only-push]: renamed repository.pushTagsOnly flag to …

2c4c32f

…release.pushTagsOnly

Fixed post-merge conflicts and updated test to use new GitRepository …

0b5228d

…constructor

[conditional-tag-only-push]: Updated docs to reflect change in pushTa…

fa21a1f

…gsOnly CLI flag

adamdubiel added a commit that referenced this pull request May 26, 2015

Merge pull request #81 from erichsend/conditional-tag-only-push

b149c5e

Added repository configuration to only push tags to remote repositories

adamdubiel merged commit b149c5e into allegro:master May 26, 2015

erichsend deleted the conditional-tag-only-push branch May 27, 2015 14:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added repository configuration to only push tags to remote repositories #81

Added repository configuration to only push tags to remote repositories #81

erichsend commented May 18, 2015

coveralls commented May 18, 2015

szpak May 18, 2015

szpak commented May 18, 2015

erichsend commented May 18, 2015

adamdubiel commented May 18, 2015

erichsend commented May 18, 2015

vbuell commented May 18, 2015

coveralls commented May 18, 2015

adamdubiel May 18, 2015

erichsend May 18, 2015

adamdubiel commented May 18, 2015

coveralls commented May 18, 2015

adamdubiel commented May 18, 2015

adamdubiel May 18, 2015

coveralls commented May 19, 2015

erichsend commented May 20, 2015

coveralls commented May 21, 2015

erichsend commented May 21, 2015

adamdubiel commented May 24, 2015

adamdubiel May 24, 2015

adamdubiel commented May 24, 2015

coveralls commented May 26, 2015

erichsend commented May 26, 2015

erichsend commented May 26, 2015

coveralls commented May 26, 2015

adamdubiel commented May 26, 2015

adamdubiel commented May 26, 2015

erichsend commented May 27, 2015

		@@ -14,14 +14,19 @@ class ScmRepositoryFactory {

		private static final String GIT = 'git'

		private static final String REPOSITORY_PUSH_TAGS_ONLY_FLAG = "repository.pushTagsOnly";

Added repository configuration to only push tags to remote repositories #81

Added repository configuration to only push tags to remote repositories #81

Conversation

erichsend commented May 18, 2015

coveralls commented May 18, 2015

szpak May 18, 2015

Choose a reason for hiding this comment

szpak commented May 18, 2015

erichsend commented May 18, 2015

adamdubiel commented May 18, 2015

erichsend commented May 18, 2015

vbuell commented May 18, 2015

coveralls commented May 18, 2015

adamdubiel May 18, 2015

Choose a reason for hiding this comment

erichsend May 18, 2015

Choose a reason for hiding this comment

adamdubiel commented May 18, 2015

coveralls commented May 18, 2015

adamdubiel commented May 18, 2015

adamdubiel May 18, 2015

Choose a reason for hiding this comment

coveralls commented May 19, 2015

erichsend commented May 20, 2015

coveralls commented May 21, 2015

erichsend commented May 21, 2015

adamdubiel commented May 24, 2015

adamdubiel May 24, 2015

Choose a reason for hiding this comment

adamdubiel commented May 24, 2015

coveralls commented May 26, 2015

erichsend commented May 26, 2015

erichsend commented May 26, 2015

coveralls commented May 26, 2015

adamdubiel commented May 26, 2015

adamdubiel commented May 26, 2015

erichsend commented May 27, 2015