[JENKINS-28335] Provide a gitPublisher step. by iwarapter · Pull Request #452 · jenkinsci/git-plugin

iwarapter · 2016-12-10T12:21:14Z

I'm after some feedback with this as i'm aware there is lots of duplication but i wasn't sure of the best approach so figured i'd share the code and see what the maintainers think :)

daniel-beck · 2016-12-10T12:41:19Z

@jenkinsci/code-reviewers PTAL

daniel-beck

One compatibility breaking change.

daniel-beck · 2016-12-10T12:43:57Z

        private boolean noteReplace;

-        public String getnoteMsg() {
+        public String getNoteMsg() {


Breaks backward compatibility for a class that is public API. Did you confirm none of the ~60 plugins with dependency on the Git Plugin use these methods?

I originally changed this as i was getting:

WARNING: failed to uncoerce hudson.plugins.git.GitPublisher$NoteToPush@12647296 java.lang.UnsupportedOperationException: no public field ‘noteMsg’ (or getter method) found in class hudson.plugins.git.GitPublisher$NoteToPush

When using the snippet generator, which resulted in this snippet being generated.

gitPublisher notesToPush: [<object of type hudson.plugins.git.GitPublisher.NoteToPush>], url: 'git@github.com:iwarapter/git-plugin.git'

Interestingly the a normal freestyle job doesn't suffer from this and the jelly config is pretty much the same. Any thoughts?

Right, I understood the motivation, but it still may have broken plugins depending on the old name. Adding fixed getters makes sense in addition to keeping the old ones. They should probably be marked @deprecated as their names don't follow convention, and their Javadoc should reference the new alternatives.

Javadocs updated, non-conforming methods marked as deprecated.

daniel-beck · 2016-12-10T12:44:05Z

+/*
+ * The MIT License
+ *
+ * Copyright 2014 Jesse Glick.


Just… no.

daniel-beck · 2016-12-10T12:44:33Z

+<!--
+The MIT License
+
+Copyright 2014 Jesse Glick.


daniel-beck · 2016-12-10T12:45:14Z

@@ -0,0 +1,5 @@
+<div>
+    <p>


Unusual to have a paragraph here I think.

daniel-beck · 2016-12-10T12:45:45Z

 #
 GitSCMSource.DisplayName=Git
 GitStep.git=Git
+GitPublisherStep=Git Publisher


Key seems inconsistent with the others.

iwarapter · 2016-12-10T13:52:53Z

@daniel-beck I have fixed the requested changes, the solution i have for the api change is a dirty hack, am i missing something with the Jelly config?

iwarapter · 2016-12-14T09:09:36Z

@daniel-beck do you require any further changes?

oleg-nenashev · 2016-12-14T11:23:01Z

+         */
+        @Deprecated
        public String getnoteMsg() {
            return noteMsg;


The old method should ideally invoke the new method

oleg-nenashev · 2016-12-14T11:23:48Z

+package jenkins.plugins.git;
+
+import com.google.inject.Inject;
+import hudson.*;


I would rather recommend to expand these imports at least for the production code

oleg-nenashev · 2016-12-14T11:24:29Z

+
+  @DataBoundConstructor
+  public GitPublisherStep(String url) {
+    this.url = url;


Why not setter as well?

oleg-nenashev · 2016-12-14T11:26:01Z

+  }
+
+  @DataBoundSetter
+  public void setBranch(String branch) {


I would recommend to make these setters @Restricted(NoExternalUse.class). Otherwise it worth to mention in Javadoc that invocation of these setters does not persist data to the disk.

oleg-nenashev · 2016-12-14T11:27:28Z

+      tagsToPush = new ArrayList<GitPublisher.TagToPush>();
+    }
+
+    return tagsToPush;


Almost 🐛 . IMHO all the lists should be returned as unmodifyable lists to prevent the exposure of the internal implementation

oleg-nenashev · 2016-12-14T11:28:56Z

+    if (isPushTags()) {
+      for (final GitPublisher.TagToPush t : tagsToPush) {
+        if (t.getTagName() == null)
+          throw new AbortException("No tag to push defined");


NIT: Missing brackets in single-line ifs

oleg-nenashev · 2016-12-14T11:30:31Z

+        if (t.getTargetRepoName() == null)
+          throw new AbortException("No target repo to push to defined");
+
+        final String tagName = environment.expand(t.getTagName());


Formally the checks above do not prevent the return value from being null here. E.g. in the case of the parallel config change. I would recommend assigning the method's return value to a local variable before the check and tthen using this variable in the code

oleg-nenashev · 2016-12-14T11:31:14Z

+              throw new AbortException("Tag " + tagName + " already exists and Create Tag is specified, so failing.");
+            }
+
+            if (tagMessage.length() == 0) {


StringUtils.isBlank() to cover more cases like empty strings

oleg-nenashev · 2016-12-14T11:31:48Z

+
+  @DataBoundSetter
+  public void setCredentialsId(String credentialsId) {
+    this.credentialsId = Util.fixEmpty(credentialsId);


fixEmptyAndTrim to prevent whitespace-only strings?

oleg-nenashev · 2016-12-14T11:32:28Z

+          listener.getLogger().println("Pushing tag " + tagName + " to repo "
+            + targetRepo);
+
+          remoteURI = remote.getURIs().get(0);


🐛 Missing check of the array size

oleg-nenashev · 2016-12-14T11:33:56Z

+        if (b.getTargetRepoName() == null)
+          throw new AbortException("No branch repo to push to defined");
+
+        final String branchName = environment.expand(b.getBranchName());


same as above

oleg-nenashev · 2016-12-14T11:34:53Z

+
+            </table>
+            <div align="right">
+                <input type="button" value="Delete Note" class="repeatable-delete" style="margin-left: 1em;" />


No localization in value

Hi @oleg-nenashev i'm not very familiar with the jelly config stuff, whats the best way to achieve this?

Hi @iwarapter . There is a Wiki page for it: https://wiki.jenkins-ci.org/display/JENKINS/Internationalization

oleg-nenashev

Sorry for not using the review mode :( . I think some logic processing needs to be fixed in order to make the implementation more robust against non-success paths

iwarapter · 2017-01-26T14:06:18Z

Hi @daniel-beck @oleg-nenashev i have just come back from holiday, what are the remaining blockers for this PR?

MarkEWaite · 2017-01-26T14:18:35Z

They need to review your changes and mark them as "accepted", then the plugin maintainer (me) needs to review the request, create integration tests to confirm it is working as expected, then merge it to the master branch and release it.

I'm currently evaluating a proposal to add large file support to the plugin. That will need at least 1-2 more weeks before I'll evaluate what's next on my review list.

You could speed the review process by creating the integration tests as well, you can model them after the tests in my lts-with-plugins docker instance, or you can submit a pull request as a new branch to my jenkins-bugs git repository.

You could also speed the review process by encouraging others to use a pre-release version of the plugin, and have them post their experiences in this pull request.

iwarapter · 2017-01-27T12:46:31Z

Hi @MarkEWaite thanks for your feedback, with regards to the integration tests, do you want a PR on the its-with-plugins or add to this PR?

iwarapter · 2017-04-07T20:01:43Z

Hi guys, any update on this?

daniel-beck

Haven't really been able to look at this in detail as I'm not very familiar with this plugin, so my comments may be irrelevant.

daniel-beck · 2017-04-23T22:00:17Z

+    }
+    String buildDuration = run.getDurationString().replaceAll("and counting", "");
+
+    input = input.replaceAll("\\$BUILDRESULT", buildResult);


Should use the utility methods in core to do variable replacement (unless I'm missing something).

Also seems like a limited use case, what is this for exactly? This is a Pipeline step, why need environment variables at all?

daniel-beck · 2017-04-23T22:02:24Z

+
+    <f:entry field="notesToPush" title="${%Notes}" description="${%Notes to push to remote repositories}">
+
+        <f:repeatable field="notesToPush" add="${%Add Note}">


Is there a reason this isn't covered by tests?

…p push notes functionality (i don't use or know how to properly test this)

# Conflicts: # src/main/resources/hudson/plugins/git/Messages.properties

jglick · 2017-05-31T21:43:40Z

As I noted in JIRA, I think this is the wrong approach. You can already use withCredentials etc. to run git commands of your choice; if anything is needed, it is just a more convenient wrapper. Similar to the concept of the pipeline-maven plugin, which sets some useful environment variables and then steps out of your way.

iwarapter · 2017-05-31T22:14:25Z

Hi @jglick thanks for your feedback, i have been working with a some of the workarounds on the JIRA ticket, mainly the sshagent approach for Linux (bug on windows prevents this). However i find myself constantly always wrapping the same commands in isUnix blocks then sh/bat accordingly, which is really bloating my global libraries and i was after a more cleaner solution.

gitPublisher( url: 'ssh:etc', tagsToPush: [tagName: '1.0-FINAL'])

if(isUnix()){
  sshagent([credentialsId]) {
        sh("git tag -a -f -m 'msg' tag")
        sh("git -c core.askpass=true push origin tag")
    }
}
else {
  //this doesnt work https://issues.jenkins-ci.org/browse/JENKINS-28279
  sshagent([credentialsId]) {
        bat("git tag -a -f -m 'msg' tag")
        bat("git -c core.askpass=true push origin tag")
    }
}

I work with many different teams utilizing a large mix of agents so try to keep my scripts as node agnostic as possible to avoid bottlenecks.

A pipeline-maven style solution for git would equally fall into the trap of isUnix if else etc.

Appreciate any feedback/suggestions!

jglick · 2017-06-01T20:57:45Z

i find myself constantly always wrapping the same commands in isUnix blocks then sh/bat accordingly

Well, this is solvable with a simple helper function, or an integration with the xshell plugin, or some other generic solution that would apply to any more or less cross-platform command, not just git.

TheDukeDK · 2018-04-17T10:07:11Z

What is the status of this? Will there be a git publisher step for pipeline or has this been dropped?

MarkEWaite · 2018-04-18T02:29:02Z

I don't plan to merge this proposed change. I agree with @jglick that this is better done from a shell wrapper in Pipeline, rather than adding more complexity to the git plugin.

iwarapter · 2018-04-18T10:21:58Z

do you know if any such plugin exists? i assume it would be something like this (but for git ofc) https://github.com/openshift/jenkins-client-plugin

[JENKINS-28335] Provide a gitPublisher step.

5c6618d

daniel-beck requested changes Dec 10, 2016

View reviewed changes

Refactor from peer review.

23c863a

Update javadocs following peer review.

4424472

oleg-nenashev reviewed Dec 14, 2016

View reviewed changes

oleg-nenashev requested changes Dec 14, 2016

View reviewed changes

Refactored after peer review.

b2af122

daniel-beck reviewed Apr 23, 2017

View reviewed changes

iwarapter added 2 commits May 31, 2017 00:44

jenkinsci#452 refactored after peer review, removed the publisher ste…

ec9f489

…p push notes functionality (i don't use or know how to properly test this)

Merge branch 'master' of github.com:jenkinsci/git-plugin

8d080fd

# Conflicts: # src/main/resources/hudson/plugins/git/Messages.properties

MarkEWaite closed this Apr 18, 2018


		<f:entry field="notesToPush" title="${%Notes}" description="${%Notes to push to remote repositories}">

		<f:repeatable field="notesToPush" add="${%Add Note}">

Uh oh!

Conversation

iwarapter commented Dec 10, 2016

Uh oh!

daniel-beck commented Dec 10, 2016

Uh oh!

daniel-beck left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iwarapter commented Dec 10, 2016

Uh oh!

iwarapter commented Dec 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oleg-nenashev left a comment

Choose a reason for hiding this comment

Uh oh!

iwarapter commented Jan 26, 2017

Uh oh!

MarkEWaite commented Jan 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iwarapter commented Jan 27, 2017

Uh oh!

iwarapter commented Apr 7, 2017

Uh oh!

daniel-beck left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jglick commented May 31, 2017

Uh oh!

iwarapter commented May 31, 2017

Uh oh!

MarkEWaite commented Jan 26, 2017 •

edited

Loading