AGDIGGER-59 trigger jenkins job by aliok · Pull Request #2 · aerogear-attic/aerogear-digger-java-client

aliok · 2016-12-09T08:45:58Z

Implemented public long build(String jobName, long timeout) in DiggerClient
Introduced SLF4J-LOG4J bridge as some dependency was using SLF4J and logs were gone to NoOpLogger
Added .editorconfig file matching current style. Will change it later to match Java conventions

Checked in as 2 separate commits: 1 for styling/formatting. 1 for the actual work.

aliok · 2016-12-09T08:47:35Z

I tested it like this:

public static void main(String[] args) throws URISyntaxException, DiggerClientException {
    DiggerClient client = new DiggerClient(new JenkinsAuth("JENKINS_URL","username","password"));
    final String jobName = "aliok-dec09-" + UUID.randomUUID().toString();
    client.createJob(jobName, "https://github.com/wtrocki/helloworld-android-gradle.git", "master");
    final long buildNumber = client.build(jobName, 10*1000);
    System.out.println(buildNumber);
  }

Please note that we use some features in Jenkins-Java-client 3.7-SNAPSHOT.
So, you need to clone https://github.com/jenkinsci/java-client-api.git and build it locally.

git clone https://github.com/jenkinsci/java-client-api.git
cd jenkins-client
mvn install

aliok · 2016-12-09T08:49:26Z

@matzew @wtrocki @ziccardi ping

matzew · 2016-12-09T09:24:47Z

            <groupId>com.offbytwo.jenkins</groupId>
            <artifactId>jenkins-client</artifactId>
-            <version>0.3.6</version>
+            <version>0.3.7-SNAPSHOT</version>


Do they have a CI running somewhere, where we can suck down the dependency ?
Not a blocker - just asking

@matzew good point!
I looked now, but couldn't find anything. I checked the pom.xml files in the source code to see some repository reference. There is one, but that's not the snapshot repository.
No mention of CI.

@khmarbaise Where can we suck Jenkins-Java-client SNAPSHOT? Unfortunately, I haven't still got the approval to post to mailing list (https://groups.google.com/d/forum/java-client-api).
As a quick introduction: we are building a mobile application build farm on top of Jenkins 😎

asked in mailing list: https://groups.google.com/forum/#!topic/java-client-api/boD3H2KCdFg

wtrocki · 2016-12-09T09:33:09Z

+      LOG.debug("Exception while connecting to Jenkins", e);
+      throw new DiggerClientException(e);
+    } catch (InterruptedException e) {
+      LOG.debug("Exception while sleeping between checks", e);


Can we rephrase: "Exception while sleeping between checks" - It's not clear what that means for me.

wtrocki · 2016-12-09T09:41:02Z

+
+      if (queueItem.isCancelled()) {
+        LOG.debug("Queue item is cancelled. Returning -1");
+        return -1;


I do not like this because we introducing magic number and missing actual problem here.
Best to introduce some object (model) that would contain build number + some info about this build. If something is wrong build number would be empty.
Alternative is to use Long and return null or provide specific client exception (Subclass our exception)

See my other comment: #2 (comment)

wtrocki · 2016-12-09T09:44:42Z

+      if (queueItem.isCancelled()) {
+        LOG.debug("Queue item is cancelled. Returning -1");
+        return -1;
+      } else if (queueItem.isStuck()) {


This probably would require us to return some exception to the client - job needs to be retrgered. I do not remember where item can be stuck, but it this is permanent we should let clients now.

@wtrocki good point. This is a combined answer for the comment above #2 (comment) and this comment.

I don't like subclassing the client exception. That would mean some casting and instanceof calls.

Lets use this model then:

class BuildStatus{ STATE state; int buildNumber; } enum STATE{ BUILDING, QUEUE_ITEM_CANCELLED, QUEUE_ITEM_STUCK, TIMED_OUT ... }

class DiggerClient{ ... public BuildStatus build(String jobName, long timeout) throws DiggerClientException{...} }

Method will only throw exception in case of

thread interruption during sleep calls

connection problem with Jenkins

matzew · 2016-12-09T09:47:08Z

@@ -1,24 +1,23 @@
 package com.redhat.digkins.services;


Let's use org.aerogear.*** as package for all of this

let's do the package change in a SEPARATE PR

matzew · 2016-12-09T09:47:41Z

unit tests ?

wtrocki · 2016-12-09T09:48:16Z

@aliok Would be good to paste your example into README.md (create job is already there)

wtrocki · 2016-12-09T17:56:56Z

Could not review change because of missing artifact:
Failed to execute goal on project jenkins-client: Could not resolve dependencies for project groupId:jenkins-client:jar:1.0.0: Could not find artifact com.offbytwo.jenkins:jenkins-client:jar:0.3.7-SNAPSHOT in jcenter (https://jcenter.bintray.com/) -> [Help 1]

I needed to build it locally.
Do you have some specific profile configured?

wtrocki · 2016-12-11T17:47:51Z

PR extended in aliok#1

aliok · 2016-12-12T06:38:28Z

Do you have some specific profile configured?
@wtrocki You need to build it locally and install into your local repo.

git clone https://github.com/jenkinsci/java-client-api.git
cd jenkins-client
mvn install

aliok · 2016-12-12T08:26:28Z

@wtrocki @matzew @josemigallas
requesting a review...

updates:

returning an object instead of plain number when triggering a build
unit test for the service

notes:

couldn't write unit tests for the class DiggerClient as services are currently instantiated in the methods (and I don't want to introduce PowerMock for stubbing constructors).
the unit test takes a while because of delays (Thread.sleep).

follow ups that I am gonna do immediately

convert services to singletons and pass them to client
make the delays parametrizable in services
write unit test for DiggerClient

How to test:

mvn clean test

Note that you need to build Jenkins-Java-client and install it locally as described in previous comments.

josemigallas · 2016-12-12T08:18:37Z

+  private final static String GIT_REPO_BRANCH = "GIT_REPO_BRANCH";

-  private final static String GIT_REPO_URL = "GIT_REPO_URL", GIT_REPO_BRANCH = "GIT_REPO_BRANCH";
+  private JenkinsServer jenkins;


Should this be final as in DiggerClient?

josemigallas · 2016-12-12T08:21:16Z

+   * @param gitBranch git repository branch (default branch used to checkout source code)
   */
  public void create(String name, String gitRepo, String gitBranch) throws IOException {
    JtwigTemplate template = JtwigTemplate.classpathTemplate("templates/job.xml");


I strongly recommend to extract hardcoded strings into a constant field, this way it can be found, checked and modified easier when needed.

josemigallas · 2016-12-12T08:24:05Z

+  /**
+   * How long should we wait before we start checking the queue item status.
+   */
+  public static final long FIRST_CHECK_DELAY = 5 * 1000L;


Just a suggestion: the time unit should be always explicit. In this case I believe they are millis so I would either name it FIRST_CHECK_DELAY_MILLIS or include it in comment /** [...] to queue item status, in milli seconds. */

josemigallas · 2016-12-12T08:26:07Z

+  public static final long POLL_PERIOD = 2 * 1000L;
+
+
+  private JenkinsServer jenkins;


Should it be final?

aliok · 2016-12-12T09:18:47Z

Merged this one.

@josemigallas your concerns are done in a separate PR

wtrocki · 2016-12-12T09:27:01Z

Reviewed. Looks ok.

couldn't write unit tests for the class DiggerClient as services are currently instantiated in the methods (and I don't want to introduce PowerMock for stubbing constructors).

DiggerClient should not have any logic and just pass all execution parameters to services so I would not bother too much

aliok · 2016-12-12T09:31:41Z

@wtrocki on a second thought... you're right. I won't write tests for DiggerClient class

aliok added 2 commits December 7, 2016 09:26

Some minor layout and doc fixes

dbfb3a7

Functionality to trigger a build

6c2d628

matzew reviewed Dec 9, 2016

View reviewed changes

wtrocki reviewed Dec 9, 2016

View reviewed changes

matzew reviewed Dec 9, 2016

View reviewed changes

Return model instead of just number when triggering a build

b340fc4

josemigallas reviewed Dec 12, 2016

View reviewed changes

aliok merged commit 795eb04 into aerogear-attic:master Dec 12, 2016

aliok mentioned this pull request Dec 12, 2016

AGDIGGER-59 trigger jenkins job - 2 #3

Merged

aliok deleted the AGDIGGER-59-trigger-jenkins-job branch May 30, 2017 08:45

		public static final long POLL_PERIOD = 2 * 1000L;


		private JenkinsServer jenkins;

Uh oh!

Conversation

aliok commented Dec 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aliok commented Dec 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aliok commented Dec 9, 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!

aliok Dec 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

matzew commented Dec 9, 2016

Uh oh!

wtrocki commented Dec 9, 2016

Uh oh!

wtrocki commented Dec 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wtrocki commented Dec 11, 2016

Uh oh!

aliok commented Dec 12, 2016

Uh oh!

aliok commented Dec 12, 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!

aliok commented Dec 12, 2016

Uh oh!

wtrocki commented Dec 12, 2016

Uh oh!

aliok commented Dec 12, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aliok commented Dec 9, 2016 •

edited

Loading

aliok commented Dec 9, 2016 •

edited

Loading

aliok Dec 9, 2016 •

edited

Loading

wtrocki commented Dec 9, 2016 •

edited

Loading