Update VSTS PR Jenkins build status back to VSTS #175

smile21prc · 2017-09-18T21:17:45Z

Update VSTS PR Jenkins build status back to VSTS.

Re-target the PR smile21prc#2 to official plug-in branch.

Update VSTS PR Jenkins build status back to VSTS

More changes.

Remove redudant API

Add support of reporting build status of multi-jobs triggered by one PR

Add support of showing customized run config in VSTS PR Status UI.

Add a few null check.

Remove unnecessary status update.

Fixed status display typo

Add jelly support

…pdate. Add a seperate checkbox for PR trigger, and fully enable its status update.

Remove redundant entry in pom

smile21prc · 2017-09-20T01:41:34Z

+@mmitche @jpricketMSFT
Based on @mmitche's suggestion, I've made added changes below on top of previous PRs:

Added a separate checkbox for PR Trigger, so customers can choose to subscribe notifications for either TFS push or TFS PR commit, or both.
Changed to use textbox from above UIs to specify names showing in VSTS status, instead of composing them with random customized tags like AGroup.
Made sure only runs kicked off by PR commit will show status update in corresponding PRs, while those runs kicked off code push will NOT.
Enabled CHECKSTYLE in multiple sources and fixed their formats.

smile21prc · 2017-09-20T18:03:34Z

Deployed the plugin to https://dotnet-vsts.westus2.cloudapp.azure.com, which worked well -- runs are correctly triggered by both PR commit and code push based on their configurations.

mmitche · 2017-09-20T22:47:30Z

tfs/src/main/java/hudson/plugins/tfs/TeamEventsEndpoint.java

+        List<T> triggerList = new ArrayList<>();
+        if (job instanceof ParameterizedJobMixIn.ParameterizedJob) {
+            final ParameterizedJobMixIn.ParameterizedJob pJob = (ParameterizedJobMixIn.ParameterizedJob) job;
+            for (final Trigger trigger : pJob.getTriggers().values()) {


Is there not a getTriggers on non ParameterizedJobs?

Nop, I don't see one in its member list, if a job isn't a ParameterizedJob:

mmitche · 2017-09-20T22:48:49Z

tfs/src/main/java/hudson/plugins/tfs/TeamPushCause.java

@@ -12,19 +12,22 @@
 public class TeamPushCause extends SCMTriggerCause {

    private final String pushedBy;
+    private final String runConfig;


Can you refactor this name to "context" just for consistency with other plugins that do similar things?

Sure, refactored it "context".

mmitche · 2017-09-20T22:50:57Z

tfs/src/main/java/hudson/plugins/tfs/TeamPushTrigger.java

@@ -48,6 +49,9 @@ public TeamPushTrigger(final Job<?, ?> job) {
        this.job = job;


In a future PR we should figure out whether this constructor can be removed.

Sure, will try that out in the PR to add refspec param into TeamPushTrigger.

Okay sounds good. It might be good for the trigger to inject a parameter for both the refspec and the hash. That way the same job can be used for both PRs and pushes if desired.

Thanks Matt. I tried multiple approaches and chose the environment contributor approach which worked locally. Now both values (vstsRefspec and vstsBranch) are shown in the "Parameters" tab on build page, and it's loadable in triggered builds.

mmitche · 2017-09-20T22:52:28Z

tfs/src/main/java/hudson/plugins/tfs/listeners/JenkinsRunListener.java

+                }
+            }
+        }
+        return "Jenkins CI build";


Can you pull this string out into a static constant somewhere?

Good suggestion. Refactored it as a static constant in JenkinsRunListener class:
private static final String DEFAULT_RUN_CONTEXT = "Jenkins CI build";

mmitche · 2017-09-20T22:53:35Z

tfs/src/main/java/hudson/plugins/tfs/model/AbstractHookEvent.java

+    }
+
+    private Boolean repoMatch(final GitCodePushedEventArgs gitCodePushedEventArgs, final Job job) {
+        if (job instanceof WorkflowJob) {


I assume this work for non workflow jobs?

Good catch. Non-workflow jobs do repo matching check first, and calls triggerJob() only if it matches (in pullOrQueueFromEvent()), but we do need a var in triggerJob() to know the status of "this is a non-workflow job whose repo already matches).

Added a new param "final Boolean repoMatch" to triggerJob() to signal that status.

mmitche · 2017-09-20T22:54:06Z

tfs/src/main/resources/hudson/plugins/tfs/TeamPRPushTrigger/TeamPollingAction/index.jelly

+<!--
+The MIT License
+
+Copyright (c) 2004-2009, Sun Microsystems, Inc., Kohsuke Kawaguchi


Copyright header should be an MS copyright.

Removed the whole file.

mmitche · 2017-09-20T22:54:55Z

tfs/src/main/resources/hudson/plugins/tfs/TeamPRPushTrigger/TeamPollingAction/index.jelly

@@ -0,0 +1,45 @@
+<!--


What's the reason for this file?

It's cloned from TeamPushTrigger. Turns out it's redudant for TeamPRPushTrigger -- it worked fine after removing it. So I've removed the whole file.

mmitche · 2017-09-20T22:55:49Z

tfs/src/main/resources/hudson/plugins/tfs/TeamPRPushTrigger/config.jelly

+<?jelly escape-by-default='true'?>
+<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form">>
+  <f:entry title="Job Context" field="jobContext">
+    <f:textbox default="Jenkins CI build" />


CI doesn't necessarily imply PR. Can you change this to "Jenkins PR Build"?

Sure, changed.

Address review comments.

smile21prc · 2017-09-20T23:44:30Z

Thanks for the review @mmitche, and I've addressed all your comments. Can you take another look at your convenient time? Thanks.

mmitche · 2017-09-21T16:33:07Z

tfs/src/main/java/hudson/plugins/tfs/JenkinsEventNotifier.java

+                }
+            }
+        } catch (final Exception e) {
+            e.printStackTrace();


These errors here should be sent to the logger (https://github.com/jenkinsci/tfs-plugin/pull/175/files#diff-462c46539e78a70b95caa4cf430ecbd7R39) so they are easy to find and filter

mmitche · 2017-09-21T16:33:13Z

tfs/src/main/java/hudson/plugins/tfs/JenkinsEventNotifier.java

+            final TeamRestClient client = new TeamRestClient(collectionUri);
+            client.addPullRequestStatus(gitCodePushedEventArgs, status);
+        } catch (MalformedURLException e) {
+            e.printStackTrace();


These errors here should be sent to the logger (https://github.com/jenkinsci/tfs-plugin/pull/175/files#diff-462c46539e78a70b95caa4cf430ecbd7R39) so they are easy to find and filter

mmitche · 2017-09-21T16:33:16Z

tfs/src/main/java/hudson/plugins/tfs/JenkinsEventNotifier.java

+        } catch (MalformedURLException e) {
+            e.printStackTrace();
+        } catch (IOException e) {
+            e.printStackTrace();


These errors here should be sent to the logger (https://github.com/jenkinsci/tfs-plugin/pull/175/files#diff-462c46539e78a70b95caa4cf430ecbd7R39) so they are easy to find and filter

mmitche · 2017-09-21T16:34:53Z

tfs/src/main/java/hudson/plugins/tfs/TeamPushTrigger.java

@@ -48,6 +49,9 @@ public TeamPushTrigger(final Job<?, ?> job) {
        this.job = job;


Okay sounds good. It might be good for the trigger to inject a parameter for both the refspec and the hash. That way the same job can be used for both PRs and pushes if desired.

mmitche · 2017-09-21T16:36:30Z

tfs/src/main/java/hudson/plugins/tfs/listeners/JenkinsRunListener.java

+        return new JSONObject();
+    }
+
+    private String getRunContext(final Run run) {


Why is this code here workflow job specific?

Good catch. It shouldn't. I've changed it to be applied to all jobs.

Address review comments.

Pass additional params to Jenkins build for PR job auto-gen

Renaming to vstsBranchOrCommit

smile21prc · 2017-09-26T01:53:15Z

@mmitche, any further comments on this PR?

Add constructor to allow creating a job trigger with job context param.

mosabua · 2017-09-26T19:49:44Z

When this is merged please ensure to do a squash merge to ensure a relatively clean history.

smile21prc · 2017-09-26T19:57:49Z

@mosabua , sure will.

smile21prc · 2017-09-26T20:18:22Z

Hi, @jpricketMSFT, seems I don't have enough access to add you as the reviewer of this PR. There are a bunch of changes after your previous sign-off. If you've got any additional comments, please feel free to let me know. Thanks.

I've deployed it to https://dotnet-vsts.westus2.cloudapp.azure.com/job/DevDiv_dotnet-linker, and verified both PR and Push trigger & VSTS status update worked well -- https://dotnet-vsts.westus2.cloudapp.azure.com/job/DevDiv_dotnet-linker/job/TestJobs/.

Added support of branch-specific triggering for both PR and code push

smile21prc · 2017-09-27T06:19:18Z

@mmitche, added support on branch-specific triggering.

PR merge jobs now are only triggered when PR happens in the branch list of PR trigger UI:

While code push jobs are triggered when code push happens in a branch matching one of its Git branches:

jpricket

I don't see any major problems. This is a large PR, so we will do extra regression testing before releasing these changes to make sure that the other plugin features are not affected.

smile21prc · 2017-09-27T15:59:56Z

Thanks @jpricketMSFT .

smile21prc added 12 commits June 23, 2017 15:58

Merge remote-tracking branch 'refs/remotes/jenkinsci/master'

d11525f

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

8d33bd4

Merge remote-tracking branch 'upstream/master'

bb6d9c9

Merge remote-tracking branch 'upstream/master'

e599ec7

Update VSTS PR Jenkins build status back to VSTS

b92d2a9

Update VSTS PR Jenkins build status back to VSTS

More changes.

abf475b

More changes.

Remove redudant API

45f897e

Remove redudant API

Add support of reporting build status of multi-jobs triggered by one PR

cc9adb2

Add support of reporting build status of multi-jobs triggered by one PR

Add support of showing customized run config in VSTS PR Status UI.

6628c90

Add support of showing customized run config in VSTS PR Status UI.

Add a few null check.

4d30307

Add a few null check.

Remove unnecessary status update.

eb8fad7

Remove unnecessary status update.

Fixed status display typo

6e33741

Fixed status display typo

smile21prc mentioned this pull request Sep 18, 2017

Update VSTS PR Jenkins build status back to VSTS smile21prc/tfs-plugin#2

Closed

smile21prc added 3 commits September 19, 2017 11:31

Add jelly support

a047e8d

Add jelly support

Add a seperate checkbox for PR trigger, and fully enable its status u…

614d260

…pdate. Add a seperate checkbox for PR trigger, and fully enable its status update.

Remove redundant entry in pom

7ee55be

Remove redundant entry in pom

mmitche reviewed Sep 20, 2017

View reviewed changes

mmitche suggested changes Sep 20, 2017

View reviewed changes

Address review comments.

e617e3b

Address review comments.

mmitche suggested changes Sep 21, 2017

View reviewed changes

smile21prc added 3 commits September 21, 2017 19:17

Address review comments.

032b538

Address review comments.

Pass additional params to Jenkins build for PR job auto-gen

8de1f7a

Pass additional params to Jenkins build for PR job auto-gen

Renaming to vstsBranchOrCommit

868c8ba

Renaming to vstsBranchOrCommit

Add constructor to allow creating a job trigger with job context param.

530bb4b

Add constructor to allow creating a job trigger with job context param.

Added support of branch-specific triggering for both PR and code push

d66e61b

Added support of branch-specific triggering for both PR and code push

jpricket self-requested a review September 27, 2017 13:51

jpricket assigned jpricket and unassigned jpricket Sep 27, 2017

jpricket approved these changes Sep 27, 2017

View reviewed changes

jpricket merged commit 102f112 into jenkinsci:master Sep 27, 2017

smile21prc deleted the pr_status branch September 27, 2017 17:54

		@@ -48,6 +49,9 @@ public TeamPushTrigger(final Job<?, ?> job) {
		this.job = job;

Update VSTS PR Jenkins build status back to VSTS #175

Update VSTS PR Jenkins build status back to VSTS #175

Conversation

smile21prc commented Sep 18, 2017

smile21prc commented Sep 20, 2017

smile21prc commented Sep 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smile21prc Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smile21prc commented Sep 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smile21prc commented Sep 26, 2017

mosabua commented Sep 26, 2017

smile21prc commented Sep 26, 2017

smile21prc commented Sep 26, 2017 • edited Loading

smile21prc commented Sep 27, 2017 • edited Loading

jpricket left a comment

Choose a reason for hiding this comment

smile21prc commented Sep 27, 2017

smile21prc Sep 20, 2017 •

edited

Loading

smile21prc commented Sep 26, 2017 •

edited

Loading

smile21prc commented Sep 27, 2017 •

edited

Loading