Initial proof-of-concept code for open source build module #1

CameronDiver · 2017-01-25T15:56:24Z

The following code is meant as a proof of concept to supplement the spec that is currently in review. Any issues that come up in this code review will be reflected in the spec (and vice versa), and then once the spec is approved we can move forward, with the further milestones and features.

Currently we have a simple build function which handles the streaming of data to/from the docker daemon, a simple plugin based architecture which contains the bare minimum to get a build to complete, and an example program using the architecture.

The example program resolves Dockerfile.template files (by assuming nuc etc for %%RESIN_...%% macros) midstream, using the hooks provided, and proceeds to build the project.

Simple package.json and tsconfig.json files have been created, but given my unfamilarity with typescript they're probably not correct, @Page- I could do with your input on these.

* Added Apache-2.0 license * Added .gitignore * Added tsconfig.json Signed-off-by: Cameron Diver <cameron@resin.io>

* Create plugin interface * Created Builder class to interface with docker daemon and call plugins * Create example application which resolves a Dockerfile.template project and streams it to docker to be built * Add utils module with docker info extraction functions Signed-off-by: Cameron Diver <cameron@resin.io>

CameronDiver · 2017-01-25T16:53:42Z

Also link to spec: https://github.com/resin-io/hq/pull/567

pimterry

I don't know much about how the builder works, so I'll leave comments on the details of that to others (although do shout if it'd be useful for me to jump in there too). I've sprinkled a few TypeScript comments around though.

It looks like tsconfig.json is already on master, but I can't see package.json anywhere? That should definitely come in 😄. A few comments on tsconfig:

I think we're settling on noImplicitAny (which will fail the compile if it ever can't accurately infer a value) and strictNullChecks (which forces you to to declare which variables can be null, and check nullable values if you want to them to go where they might explode) as standard settings to enable.
You haven't committed the build output here. I'm not sure what the 'right' solution is - the CS projects all seem to have build output included, some newer projects seem less sure - but it'd be nice to explicitly settle on whichever option we're going for and do it everywhere.
You should specify a target. It defaults to es3, which is probably way below what you want. Letting typescript use modern features will make the resulting code a slightly faster and smaller and simpler (it won't have to include workarounds), and there's also quite a few language features it will refuse to compile if you don't specify a target that supports them. es5 is probably reasonable.
You're already outputting declarations, but it'd be nice to do sourcemaps too for easier debugging (just set sourceMap: true), and to reference the declarations in types in your package.json, if you haven't already.

pimterry · 2017-01-27T16:54:46Z

src/example/app.ts

@@ -0,0 +1,83 @@
+const tar = require('tar-stream');
+const fs = require('fs');
+const _ = require('lodash');


We should try and use real imports where we can (i.e. import fs = require('fs'), not const) as they'll let use real type definitions, if we can get them. You can mostly pull type definitions from npm, e.g. npm install --save-dev @types/lodash. fs is part of node (@types/node), doesn't look like tar-stream has definitions, so that one could stay as-is (unless you're feeling super keen).

pimterry · 2017-01-27T16:55:03Z

src/example/app.ts

+ * @returns {ReadableStream}
+ *	A stream which when read, produces the project tar archive
+ */
+let getProjectStream = (inputStream : NodeJS.ReadableStream) : NodeJS.ReadableStream => {


Nit: function definitions should probably be const.

I hadn't considered that actually, good spot.

pimterry · 2017-01-27T16:58:48Z

src/example/project-type.ts

+		// obviously this will depend on the project type, so just support Dockerfile.template for POC
+		if(header.name === './Dockerfile.template') {
+			//this.templateContent = stream.read(header.size).toString();
+			//console.log(this.templateContent);


^ Commented out code should probably go

Definitely :p

pimterry · 2017-01-27T17:01:09Z

src/index.ts

+const JSONStream = require('JSONStream');
+const duplexify = require('duplexify');
+const es = require('event-stream');
+const _ = require('lodash');


^ Quite a few of these (fs, path, lodash, event-stream) could easily have proper types too.

My initial thinking was to only require "properly" when a type is used, but given our discussion on FD and procbots, and also for arg types which hadn't really crossed my mind, I'll be changing all of these to import properly.

A practice I like from my Python days is to alphabetically sort imports, and also group them by "distance" -- i.e. platform imports first, external deps second, project locals last. I know this isn't practiced in JS in general (or Resin), but you might want to consider.

pimterry · 2017-01-27T17:02:37Z

src/index.ts

+
+Promise.promisifyAll(Docker.prototype);
+Promise.promisifyAll(fs);
+Promise.promisifyAll(tar);


I don't know about tar and Docker, but mz might be of interest if you're looking for promisified builtins like fs (and has published and promisified type definitions too).

Nice, I'll look into that.

pimterry · 2017-01-27T17:10:09Z

src/index.ts

+					pack.pipe(stream);
+					// ...and return it for reading
+					resolve(stream);
+				});


Indentation here is a little confusing - looks like the then is inside the all, but it's not.

dfunckt · 2017-01-30T12:01:55Z

src/example/app.ts

+	let extract = tar.extract();
+	let pack = tar.pack();
+
+	let proj : ProjectType = new ProjectType();


TypeScript can infer the type of the variable automatically, so the var need not be explicitly typed here:

let proj = new ProjectType();

It can probably also be const as the reference itself is immutable.

dfunckt · 2017-01-30T12:04:51Z

src/example/app.ts

+ */
+let getProjectStream = (inputStream : NodeJS.ReadableStream) : NodeJS.ReadableStream => {
+	let extract = tar.extract();
+	let pack = tar.pack();


These two can also be const. IMO it's best to always think and type const by default and only change to let if necessary.

Fully agree.

dfunckt · 2017-01-30T12:06:35Z

src/example/app.ts

+	extract.on('entry', (header, stream, next) => {
+		// If the project type handler does not require the file,
+		// add it to the archive unchanged
+		if(!proj.provideEntry(stream, header)) {


dfunckt · 2017-01-30T12:07:08Z

src/example/app.ts

+	// Send the old tar archive to be unpacked
+	inputStream.pipe(extract);
+
+


Extra new line.

dfunckt · 2017-01-30T12:08:46Z

src/example/app.ts

+	}
+};
+
+let builder = new Builder('/var/run/docker.sock');


The socket should probably be configurable instead of hardcoded here?

This is the example implementation, so in this case hardcoded is fine. This file is an example of the lib being used, in which case the socket path would be set.

dfunckt · 2017-01-30T12:10:05Z

src/example/project-type.ts

+
+	private templateContent : string = "";
+
+	public provideEntry(stream, header) : boolean {


Wouldn't it make sense to specify argument types?

It would, I just havn't gotten round to making the example code nice as it's more of a test for now. Really this PR so far is to catch the oddities of TS going forward.

dfunckt · 2017-01-30T12:14:13Z

src/index.ts

+ */
+export default class Builder {
+
+	private docker;


Yeah the type stuff, (in addition to your comment above) is not fully implemented, both on our side and the modules which we are using. The main thing to get past at the moment is what to do when type information doesn't exist.

* Included build output in lib/ * Conform to initial tslint settings - Will change * Use const where possible * Try to use correct imports all of the time * Use mz/fs instead of fs for promisified file system functions Signed-off-by: Cameron Diver <cameron@resin.io>

Signed-off-by: Cameron Diver <cameron@resin.io>

Page- · 2017-01-30T17:52:08Z

src/utils.ts

+function extractArrowMessage(message: string) : string | undefined {
+	let arrowTest = /^\s*-+>\s*(.+)/i;
+	let match;
+	if (match = arrowTest.exec(message))


Can we keep braces even for single line ifs - it's far too easy to add an extra line and forget you suddenly have to add braces

That's fine by me, @hedss we should add this to the tslint spec.

Page- · 2017-01-30T17:52:58Z

src/utils.ts

+	if (match = arrowTest.exec(message))
+		return match[1];
+	else
+		return undefined;


I personally would go for dropping semi-colons

Do you mean dropping semi-colons everywhere possible, or just for returns for example? It would be nice to formalize this, for my own and the tslint specs sake.

I'd go for dropping them from everywhere, unless explicitly required (which is almost never, and the cases I know of where they're required are a little questionable) as they just add some visual noise. I think TS would also catch the rare case due to types not matching up

Understood, thanks!

Page- · 2017-01-30T17:54:06Z

src/utils.ts

+	if (match = arrowTest.exec(message))
+		return match[1];
+	else
+		return undefined;


Is this able to be just return?

Page- · 2017-01-30T17:57:33Z

src/index.ts

+					// Add this file to the tar archive
+					// FIXME: Use streams to add to the tar archive
+					return pack.entryAsync({name: file, size: stats.size}, fs.readFileSync(relPath));
+				}))


Can you put the final ) on a new line, so that it lines up with the indentation level it starts with - I find it makes it much easier to match up starting/opening brackets and the indented blocks they contain

Page- · 2017-01-30T17:58:07Z

src/index.ts

+
+					// Add this file to the tar archive
+					// FIXME: Use streams to add to the tar archive
+					return pack.entryAsync({name: file, size: stats.size}, fs.readFileSync(relPath));


Can we get rid of the synchronous fs accesses pleases

Page- · 2017-01-30T18:04:17Z

src/index.ts

+		})
+		.catch((err: Error) => {
+			// Call the plugin's error handler
+			instance.callHook('buildFailure', [err.toString()]);


It seems like it would make sense to pass the error object itself, that way the hook would have a lot more control over how to handle it (especially as matching strings often falls down if you're working in different languages)

Page- · 2017-01-30T18:07:29Z

package.json

+    "@types/lodash": "^4.14.51",
+    "@types/mz": "0.0.30",
+    "@types/node": "^7.0.2",
+    "@types/promise": "^7.1.30"


There looks to be imports for 3 different types of promises, do we need them all?

Ah no, you're correct. We only need the es6 and bluebird promises.

Page- · 2017-01-30T18:08:06Z

src/index.ts

+const duplexify = require('duplexify');
+// Following types are available, but do not work...
+const Docker = require('dockerode');
+const es = require('event-stream');


Can this be an import, I see the types for it are in package.json?

Unfortunately I can't get them to work quite right. Myself, @pimterry and @hedss have been discussing the best way to fix this. The dockerode import at least can be fixed with DefinitelyTyped/DefinitelyTyped#14187

Page- · 2017-01-30T18:08:12Z

src/index.ts

+// Following types are available, but do not work...
+const Docker = require('dockerode');
+const es = require('event-stream');
+const JSONStream = require('JSONStream');


Can this be an import, I see the types for it are in package.json?

Page- · 2017-01-30T18:09:31Z

package.json

+  "name": "resin-docker-build",
+  "version": "1.0.0",
+  "description": "A containerised builder which interacts with the docker remote API to perform builds.",
+  "main": "lib/index.js",


As I understand it there should be an entry for the .ts types, so that people depending on this module can also benefit from typings

* Use async fs calls * Remove unnecessary promise import * formatting changes * Use rest vars for callHooks, to avoid wrapping args in [] * Return error object instead of string Signed-off-by: Cameron Diver <cameron@resin.io>

Page- · 2017-01-31T21:40:36Z

src/utils.ts

-		if(match = shaRegex.exec(extract)) {
+export function extractLayer(message: string): string | undefined {
+	const extract = extractArrowMessage(message);
+	if(extract !== undefined) {


I think there's meant to be a space before the paren here - we really need to get ts linting set up

Ah I wasn't aware that that was what we wanted (it's not in the coding standards). I've added it to the tslint now.

Page- · 2017-01-31T22:25:16Z

src/plugin.ts

+	 * This hook will be called in the event of a build failure.
+	 *
+	 * @param {string} error
+	 *	This parameter will be populated with a string representation of the error.


This comment is out of date now, same with the jsdoc type above (is there any way to have it read the type from the typescript annotations? that'd be awesome)

A little bit of research makes me think that it's not currently possible. At the very least it would be good if we had a tslint rule which would check the comments against the types in the code, which would catch things like this.

There is TypeDoc: https://github.com/TypeStrong/typedoc.

It's pretty close to JSDoc, but you just drop the parameter types entirely, and it pulls them in from the TS compiler. Very minimally documented, not well known, and I suspect it's fairly immature. The CloudFlare team are using it though - https://blog.cloudflare.com/generating-documentation-for-typescript-projects/ - and their examples in that post make for one of the better intros around.

Signed-off-by: Cameron Diver <cameron@resin.io>

Page- · 2017-02-06T22:09:52Z

src/utils.ts

+}
+
+function extractArrowMessage(message: string): string | undefined {
+	let arrowTest = /^\s*-+>\s*(.+)/i


These can both be const

Page- · 2017-02-06T22:11:10Z

src/index.ts

+	private callHook = (hook: string, ...args: any[]) : any => {
+		if (hook in this.hooks) {
+			// Spread the arguments onto the callback function
+			let fn = this.hooks[hook]


This can be const

Page- · 2017-02-06T22:52:54Z

src/index.ts

+	 *	A stream which is connected to the output of the docker daemon
+	 */
+	public buildDir(dirPath: string, buildOpts: Object): Promise<NodeJS.ReadableStream> {
+		return new Promise<NodeJS.ReadableStream>((resolve, reject) => {


There's no need to manually construct a promise here, you chain just return the chain we have just below, check out http://bluebirdjs.com/docs/anti-patterns.html#the-explicit-construction-anti-pattern

Page- · 2017-02-06T23:03:22Z

src/index.ts

+
+			const outputStream = res
+			// parse the json objects
+			.pipe(JSONStream.parse())


Do we need to catch errors here?

Will this not be propagated to the catch block of the parent promise chain?

Nope, this is a stream pipe rather than a promise chain - you need to add a separate .on 'error', (err) -> handler to each stream that can produce errors, if there isn't an error handler for a stream that produces an error then it will crash the nodejs process

Page- · 2017-02-06T23:04:46Z

src/index.ts

+			.pipe(JSONStream.parse())
+			// Don't use fat-arrow syntax here, to capture 'this' from es
+			.pipe(es.through(function(data: any) {
+				if (data.error) {


If data can be any, then this could throw if it's undefined | null

Page- · 2017-02-06T23:06:59Z

src/index.ts

+	 * Example:
+	 * new Builder('/var/run/docker.sock')
+	 */
+	constructor(dockerPath: string) {


Would it be better to have this take any valid dockerode options, or even a dockerode object directly? That way we automatically support building on remote hosts

Page- · 2017-02-06T23:10:57Z

src/index.ts

+ */
+export default class Builder {
+
+	private docker: any


Any way to type this as a dockerode instance?

Page- · 2017-02-06T23:12:54Z

.gitignore

@@ -1,3 +1,2 @@
 # Ignore javascript files


It might be worth removing the comment too ;)

Cameron Diver added 2 commits January 25, 2017 14:02

Initial commit

363fcec

* Added Apache-2.0 license * Added .gitignore * Added tsconfig.json Signed-off-by: Cameron Diver <cameron@resin.io>

CameronDiver self-assigned this Jan 25, 2017

CameronDiver requested review from dfunckt, Page-, lurch, petrosagg, shaunmulligan, lekkas and hedss January 25, 2017 15:56

CameronDiver requested a review from pimterry January 27, 2017 16:48

pimterry suggested changes Jan 27, 2017

View reviewed changes

dfunckt reviewed Jan 30, 2017

View reviewed changes

src/example/app.ts Outdated

// Send the old tar archive to be unpacked

inputStream.pipe(extract);

Copy link

Contributor

dfunckt Jan 30, 2017

Choose a reason for hiding this comment

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

Extra new line.

dfunckt reviewed Jan 30, 2017

View reviewed changes

CameronDiver force-pushed the develop branch from b5c1594 to b8df214 Compare January 30, 2017 17:20

Make types dev-deps rather than full deps

e8c323d

Signed-off-by: Cameron Diver <cameron@resin.io>

Page- suggested changes Jan 30, 2017

View reviewed changes

Addressed review comments

4accb04

* Use async fs calls * Remove unnecessary promise import * formatting changes * Use rest vars for callHooks, to avoid wrapping args in [] * Return error object instead of string Signed-off-by: Cameron Diver <cameron@resin.io>

Page- reviewed Jan 31, 2017

View reviewed changes

Updated all code to conform to new tslint settings

ecfc516

Signed-off-by: Cameron Diver <cameron@resin.io>

Page- suggested changes Feb 6, 2017

View reviewed changes

CameronDiver closed this Feb 13, 2017

CameronDiver force-pushed the master branch from 363fcec to 2948d8e Compare February 13, 2017 11:13

CameronDiver mentioned this pull request Feb 13, 2017

Initial code PR #2

Merged

		// Send the old tar archive to be unpacked
		inputStream.pipe(extract);


		private templateContent : string = "";

		public provideEntry(stream, header) : boolean {

Initial proof-of-concept code for open source build module #1

Initial proof-of-concept code for open source build module #1

Conversation

CameronDiver commented Jan 25, 2017 • edited

CameronDiver commented Jan 25, 2017

pimterry left a 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

dfunckt Jan 30, 2017 • edited

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CameronDiver commented Jan 25, 2017 •

edited

dfunckt Jan 30, 2017 •

edited