Digger minimal node client backbone #1

wtrocki · 2016-12-03T01:05:30Z

Minimal example for digger node.js client
Supported methods:

login
job creating
triggering build
fetching logs

wtrocki · 2016-12-03T22:58:57Z

matzew · 2016-12-05T09:58:45Z

lib/api/createJob.js

+module.exports = (auth, jobArgs, callback) => {
+    var options = authHelper.createJenkinsOptions(auth.url, auth.user, auth.password);
+    var jenkins = require('jenkins')(options);
+    var fileLocation = path.join(__dirname, '..', '..', 'templates', 'standard.xml');


Why do we need a Jenkins XML / template file here ?

That's just plain template to create job. We using jenkinsfiles but we would need some simple xml file to create job that would have only giturl and branch configured. Rest would come from Jenkinsfile itself.

I investigated Jenkinfile's automatic job creation and it's really powerful, but for our internal needs we would need to create job explicitly.

matzew · 2016-12-05T10:38:35Z

I am getting this, locally w/ the client:

➜  digger-node git:(31eba05) ✗ node ./bin/digkins.js login https://jenkins-digger.osm3.feedhenry.net
Username: admin
Password: ****************
bin/digkins.js login <url> [user] [password]

Options:
  --version   Show version number                                      [boolean]
  -h, --help  Show help                                                [boolean]

auth.createJenkinsOptions is not a function

matzew · 2016-12-05T10:39:13Z

node --version

v6.9.1

@wtrocki do I need different node version ?

josemigallas · 2016-12-05T10:00:21Z

README.md

+  Create jenkins job for git repository with Jenkinsfile                         
+  digkins job create <name> [repository] [branch]  
+
+  Setup jenkins credetials and login into jenkins                          


credetials -> credentials

josemigallas · 2016-12-05T10:05:04Z

lib/api/createJob.js

+            name: jobArgs.name,
+            xml: jobXML
+        }
+        jenkins.job.create(options, function (err, data) {


I'd suggest using arrow function instead, for consistency

josemigallas · 2016-12-05T10:12:39Z

lib/api/createJob.js

+ */
+module.exports = (auth, jobArgs, callback) => {
+    var options = authHelper.createJenkinsOptions(auth.url, auth.user, auth.password);
+    var jenkins = require('jenkins')(options);


IMHO I would avoid using require anywhere but at the top of the file so that you can easily identify what modules this file depends on.

josemigallas · 2016-12-05T10:18:06Z

lib/api/createJob.js

+            callback(err, data);
+        });
+    } else {
+        callback("Standard template is missing!");


Returning a promise instead of using a callback would increase readability and simplicity. I strongly recommend using them from the beginning, specially when you can use node built-in promises.

@josemigallas Let's not start flame wars here.
We had team wide discussions about how we going to use ES2016 and what rules we should have. All of this is here: https://github.com/fheng/best_practice
I'm going to add eslint support soon.

No flame wars at all, just my humble suggestion, don't get me wrong. I read already that repo and it's said nothing about the preferred use of callback/Promise. However, from the last discussion I understood that Promises are preferred.

josemigallas · 2016-12-05T10:22:53Z

lib/api/jenkinsInfo.js

@@ -0,0 +1,14 @@
+var auth = require("../util/auth")
+
+// Retrieve jenkins server information


Just an observation: the other doc is written with block format (/** ... */) but here is line format (// ...)

josemigallas · 2016-12-05T10:25:34Z

lib/commands/job.js

@@ -0,0 +1,7 @@
+exports.command = 'job <command>'
+exports.desc = 'Manage jenkins jobs'
+exports.builder = function (yargs) {


Again, here is a clearer example that the code cleanness would benefit from using arrow functions.

josemigallas · 2016-12-05T10:32:19Z

lib/commands/jobs/build.js

+exports.command = 'build <job>'
+exports.describe = 'Trigger build for Jenkins job'
+exports.builder = (yargs) => {
+    return yargs.count('verbose').alias('v', 'verbose');


IMO this would looks utterly better on a single line without return.

That should be part of the javascript styling tool - something like eslint. Going to add this soon. Any manual styling are hard to enforce and I do not want to make changes for this now until we would have automatic process in place that would fail the build.

What I mean is that it should be
yargs => yargs.count('verbose').alias('v', 'verbose');

josemigallas · 2016-12-05T10:35:38Z

lib/commands/jobs/create.js

+            }
+            return;
+        }
+            logger.info(`Job ${argv.name} created!`);


This indentation is one step further making it less readable.

josemigallas · 2016-12-05T10:46:52Z

lib/api/jenkinsInfo.js

@@ -0,0 +1,14 @@
+var authUtil = require("../util/auth")
+
+// Retrieve jenkins server information


Here the doc is written in single line format (// ...) but in the other files is with block format (/** ... */) I suggest using always the block format.

wtrocki · 2016-12-05T10:49:12Z

@matzew

No shrinkwrap used because I'm using fixed versions in package.json. So we should not have shrinkwrap here and possibly no shrinkwrap in any other projects.

Good point about node version. I would mark that we support node>=4 in package.json

matzew · 2016-12-05T10:54:27Z

Using this GH project:
https://github.com/wtrocki/helloworld-android-gradle

returns in this error:

node ./bin/digkins.js log matzew-android 94
## Starting streaming build 1 log
Started by user Jenkins Admin
Cloning the remote Git repository
Cloning repository https://github.com/wtrocki/helloworld-android-gradle
 > git init /var/lib/jenkins/jobs/matzew-android/workspace@script # timeout=10
Fetching upstream changes from https://github.com/wtrocki/helloworld-android-gradle
 > git --version # timeout=10
 > git fetch --tags --progress https://github.com/wtrocki/helloworld-android-gradle +refs/heads/*:refs/remotes/origin/*
 > git config remote.origin.url https://github.com/wtrocki/helloworld-android-gradle # timeout=10
 > git config --add remote.origin.fetch +refs/heads/*:refs/remotes/origin/* # timeout=10
 > git config remote.origin.url https://github.com/wtrocki/helloworld-android-gradle # timeout=10
Fetching upstream changes from https://github.com/wtrocki/helloworld-android-gradle
 > git fetch --tags --progress https://github.com/wtrocki/helloworld-android-gradle +refs/heads/*:refs/remotes/origin/*
Seen branch in repository origin/Android-N
Seen branch in repository origin/FH-v3.10.0
Seen branch in repository origin/FH-v3.10.1
Seen branch in repository origin/FH-v3.11.0
Seen branch in repository origin/FH-v3.11.1
Seen branch in repository origin/FH-v3.12.0
Seen branch in repository origin/FH-v3.13.0
Seen branch in repository origin/FH-v3.13.1
Seen branch in repository origin/FH-v3.14.0
Seen branch in repository origin/FH-v3.15.0
Seen branch in repository origin/FH-v3.7.0
Seen branch in repository origin/FH-v3.8.0
Seen branch in repository origin/FH-v3.8.1
Seen branch in repository origin/FH-v3.9.0
Seen branch in repository origin/FH-v3.9.1
Seen branch in repository origin/RHMAP-3243_Forms-App-PDF-CSV-Export
Seen branch in repository origin/master
Seen branch in repository origin/revert-16-Android-N
Seen branch in repository origin/update
Seen 19 remote branches
Multiple candidate revisions
Checking out Revision 51406d8f429d4c9f31057314284273c0ab02955c (origin/Android-N)
 > git config core.sparsecheckout # timeout=10
 > git checkout -f 51406d8f429d4c9f31057314284273c0ab02955c
First time build. Skipping changelog.
ERROR: /var/lib/jenkins/jobs/matzew-android/workspace@script/Jenkinsfile not found
Finished: FAILURE

So, I could fetch the log file, with the node client (yay), but there is something wrong w/ the client ?

wtrocki · 2016-12-05T11:06:45Z

@matzew - Tested, it works fine.

matzew · 2016-12-05T11:11:44Z

COOL

matzew · 2016-12-05T11:12:11Z

@josemigallas can you test this too, before we merge it ?

Feel free to test against OSM3, shared cluster

matzew · 2016-12-05T11:55:37Z

I trriggered a build, and did fetch the logs:

 node ./bin/digkins.js log matzew-android 97

First time: worked good

Now, running that above comment a second time, to fetch the logfile again, I am getting:

error: Cannot fetch build log for matzew-android

matzew · 2016-12-05T11:57:00Z

@wtrocki Not sure I get what's the number (97 in this case) means.

the build job on Jenkins itself is #3:
https://jenkins-digger.osm3.feedhenry.net/job/matzew-android/3/

wtrocki · 2016-12-05T13:00:38Z

@matzew When triggering build it's placed in the queue (there is no build number initially because build may wait a little for possible executors). 97 is just jenkins queue number that would allow you to query your request all the time. Later when your build would start you get this '#3' number and you can use it as well, but 97 number is easyer from the api point of view because you do not need to pull jenkins until job would be scheduled.

matzew · 2016-12-05T13:31:13Z

node ./bin/digkins.js log matzew-android 3 gives me no log as well

wtrocki · 2016-12-05T13:34:01Z

@matzew Only queue items are supported for the moment. I'm going to investigate this once I would deal with other comments. Thanks for really good review guys!

wtrocki · 2016-12-05T18:32:40Z

@matzew I replicated problem with logging and it looks like issue with jenkins. Some kind of variation of: https://issues.jenkins-ci.org/browse/JENKINS-27256. Jenkins removes queue item after a while, which is against their documentation. Going to check if the most recent versions have this as well and if we cannot fix it then I would migrate to different api. This looks like problem that can be resolved on jenkins rather than on client.

I have resolved all the comments. Going to merge that now as we definitely going to rewrite this after we would support different platforms.

aliok · 2016-12-06T08:58:14Z

@wtrocki number 97 in @matzew 's case is not related to the job. So, the call could be

node ./bin/digkins.js log 103

or

node ./bin/digkins.js log matzew-android 3

Fetching logs didn't work in my trial as well.

wtrocki · 2016-12-06T09:31:53Z

@aliok This was just client backbone and I explained wht this fails and how we going to resolve it. Looks like queue item is disappearing shortly after work is build, so we would need to wait until work would be queued to build (this can take time depending on other jobs running) and then use job number to retrieve job details. We can clearly inspire here by other clients like nestor:
https://github.com/cliffano/nestor

aliok · 2016-12-06T13:20:28Z

lib/util/config.js

@@ -0,0 +1,5 @@
+const Configstore = require('configstore');


+1 on configstore. didn't know that one, looks great.

wtrocki added 2 commits December 3, 2016 01:04

Digger minimal node client backbone

aadf748

Support for logs

871db65

wtrocki force-pushed the backbone branch from b439f3d to 871db65 Compare December 3, 2016 22:57

wtrocki mentioned this pull request Dec 4, 2016

Update to allow for android-sdk-install (via jenkins job) aerogear/aerogear-digger#13

Closed

wtrocki added 2 commits December 4, 2016 20:42

Added command line support

52b132d

Documentation update

31eba05

matzew reviewed Dec 5, 2016

View reviewed changes

Fix login problem

00dd80d

wtrocki force-pushed the backbone branch from 2a8793e to 00dd80d Compare December 5, 2016 10:43

josemigallas reviewed Dec 5, 2016

View reviewed changes

Grunt support

90c4d80

Removed unused grunt targets

a42a087

Fixed eslint issues

8d52967

wtrocki force-pushed the backbone branch from d831951 to 8d6ea93 Compare December 5, 2016 18:35

Addes small fixes in format

c834be8

wtrocki force-pushed the backbone branch from 8d6ea93 to c834be8 Compare December 5, 2016 18:41

wtrocki merged commit 48a0558 into aerogear:master Dec 5, 2016

wtrocki deleted the backbone branch December 5, 2016 18:46

aliok reviewed Dec 6, 2016

View reviewed changes

lib/util/config.js

@@ -0,0 +1,5 @@

const Configstore = require('configstore');

Copy link

Member

aliok Dec 6, 2016

Choose a reason for hiding this comment

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

+1 on configstore. didn't know that one, looks great.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Digger minimal node client backbone #1

Digger minimal node client backbone #1

wtrocki commented Dec 3, 2016 •

edited

wtrocki commented Dec 3, 2016

matzew Dec 5, 2016

wtrocki Dec 5, 2016 •

edited

matzew commented Dec 5, 2016

matzew commented Dec 5, 2016

josemigallas Dec 5, 2016

josemigallas Dec 5, 2016

josemigallas Dec 5, 2016

josemigallas Dec 5, 2016

wtrocki Dec 5, 2016

josemigallas Dec 5, 2016

josemigallas Dec 5, 2016

josemigallas Dec 5, 2016

josemigallas Dec 5, 2016

wtrocki Dec 5, 2016

josemigallas Dec 5, 2016 •

edited

josemigallas Dec 5, 2016

josemigallas Dec 5, 2016

wtrocki commented Dec 5, 2016

matzew commented Dec 5, 2016

wtrocki commented Dec 5, 2016

matzew commented Dec 5, 2016

matzew commented Dec 5, 2016

matzew commented Dec 5, 2016

matzew commented Dec 5, 2016

wtrocki commented Dec 5, 2016 •

edited

matzew commented Dec 5, 2016

wtrocki commented Dec 5, 2016 •

edited

wtrocki commented Dec 5, 2016 •

edited

aliok commented Dec 6, 2016 •

edited

wtrocki commented Dec 6, 2016

aliok Dec 6, 2016

		@@ -0,0 +1,14 @@
		var auth = require("../util/auth")

		// Retrieve jenkins server information

		@@ -0,0 +1,14 @@
		var authUtil = require("../util/auth")

		// Retrieve jenkins server information

Digger minimal node client backbone #1

Digger minimal node client backbone #1

Conversation

wtrocki commented Dec 3, 2016 • edited

wtrocki commented Dec 3, 2016

Choose a reason for hiding this comment

wtrocki Dec 5, 2016 • edited

Choose a reason for hiding this comment

matzew commented Dec 5, 2016

matzew commented Dec 5, 2016

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

josemigallas Dec 5, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wtrocki commented Dec 5, 2016

matzew commented Dec 5, 2016

wtrocki commented Dec 5, 2016

matzew commented Dec 5, 2016

matzew commented Dec 5, 2016

matzew commented Dec 5, 2016

matzew commented Dec 5, 2016

wtrocki commented Dec 5, 2016 • edited

matzew commented Dec 5, 2016

wtrocki commented Dec 5, 2016 • edited

wtrocki commented Dec 5, 2016 • edited

aliok commented Dec 6, 2016 • edited

wtrocki commented Dec 6, 2016

Choose a reason for hiding this comment

wtrocki commented Dec 3, 2016 •

edited

wtrocki Dec 5, 2016 •

edited

josemigallas Dec 5, 2016 •

edited

wtrocki commented Dec 5, 2016 •

edited

wtrocki commented Dec 5, 2016 •

edited

wtrocki commented Dec 5, 2016 •

edited

aliok commented Dec 6, 2016 •

edited