Add push command which starts a build on remote resin servers #868

CameronDiver · 2018-04-25T18:09:18Z

This PR adds a module which coordinates building projects using the resin remote servers. Currently this is meant to have feature parity with git push resin master.

Also added a function in patterns.ts which provides an interface to ask the user a question, and give several options.

Change-type: minor
Connects-to: #843
---- Autogenerated Waffleboard Connection: Connects to #843

pimterry · 2018-05-03T14:54:45Z

lib/actions/push.ts

+export const push: CommandDefinition = {
+	signature: 'push <application>',
+	description: 'Start a remote build',
+	help: 'TODO',


This TODO needs TODOING

pimterry · 2018-05-07T17:06:12Z

lib/actions/push.ts

+	});
+
+	if (applications == null || applications.length === 0) {
+		throw new Error(`No applications found with name: ${appName}`);


This should use exitWithExpectedError (which stops the error being reported to Sentry).

pimterry · 2018-05-07T17:10:36Z

lib/actions/push.ts

+			parameter: 'source',
+		},
+	],
+	// FIXME: We shouldn't need to type options here


The general way I've been doing this is passing the types as generic args to CommandDefinition. Something like:

export const push: CommandDefinition<{ application: string }, { source: string }> = { ...

That'll then infer the type of param & options here correctly for you.

(I have a beautiful dream that one day I'll find time to rewrite Capitano in a form that makes these types totally inferrable from the definition itself, but not today)

pimterry · 2018-05-07T17:11:47Z

lib/actions/push.ts

+
+		const app: string | null = params.application;
+		if (app == null) {
+			throw new Error('You must specify an application');


This should be an expected error too.

pimterry · 2018-05-07T17:15:18Z

lib/utils/remote-build.ts

+
+import { tarDirectory } from './compose';
+
+const DEBUG_MODE = process.env.DEBUG === '1';


Everywhere else debug is set with !!process.env.DEBUG - i.e. any set non-empty value.

pimterry · 2018-05-07T17:26:33Z

lib/utils/remote-build.ts

+			process.stderr.write('Received SIGINT, cleaning up. Please wait.\n');
+			cancelBuildIfNecessary(build).then(() => {
+				stream.end();
+				process.exit(0);


Should this be 0? I think 130 is standard for SIGINT.

Given the workflow, I don't think it matters too much, e.g. if we're here, we know we can exit straight away. That being said, 130 is more correct, so I'll change it.

pimterry · 2018-05-07T17:32:00Z

lib/utils/remote-build.ts

+		process.removeAllListeners('SIGINT');
+		process.on('SIGINT', () => {
+			process.stderr.write('Received SIGINT, cleaning up. Please wait.\n');
+			cancelBuildIfNecessary(build).then(() => {


It looks like the builder already checks for closed connections and cancels builds for us: https://github.com/resin-io/resin-builder/blob/92b078f85216d2d62408fde9a3ea87ae400c903c/src/run-build.ts#L173-L180

Can't we use that, instead of reimplementing this client-side too?

Due to the ELB, this doesn't always work. We have extra code in the git server to handle this too, and tbh I think redundancy on something like this is completely fine. If a user ctrl+c's a resin push, the last thing we want is for the build to still get deployed.

pimterry · 2018-05-07T17:40:34Z

lib/utils/remote-build.ts

+
+async function getRequestStream(build: RemoteBuild): Promise<Stream.Duplex> {
+	// Tar the directory so that we can send it to the builder
+	const tarStream = await tarDirectory(build.source);


Annoyingly relative paths seem to cause problems with pkg at times e.g. #824.

We need to path.resolve(build.source) somewhere (probably here) to get the correct absolute path and make sure we avoid this.

Ah, good to know, thanks!

pimterry · 2018-05-07T18:26:52Z

lib/utils/remote-build.ts

+		console.log('[debug] Opened builder connection. Streaming data...');
+	}
+
+	tarStream.pipe(post);


Right now, there's no output until the context directory is tarred and fully uploaded. That's not great if you have a large local directory. There was quite a bit of confusion from local push where people didn't understand what it was doing and why it took so long, because they accidentally had enormous local folders they were pushing over the network each time.

Can we add some initial output and a nice spinner while we do the upload?

pimterry · 2018-05-07T18:28:04Z

lib/utils/remote-build.ts

+}
+
+async function cancelBuildIfNecessary(build: RemoteBuild): Promise<void> {
+	if (build.running && build.releaseId != null) {


Why do we need the separate running flag? Can't we just check if we've got a release id?

ghost · 2018-05-09T11:35:30Z

@CameronDiver, status checks have failed for this PR. Please make appropriate changes and recommit.

pimterry

The build's failing btw because you haven't rebuilt the docs or prettified. npm run build will make sure everything's 100% up to date.

pimterry · 2018-05-09T12:26:22Z

lib/actions/push.ts

+`This command can be used to start a build on the remote resin.io cloud
+builders. The given source directory will be added to a tarfile, which
+will be sent to the resin.io builder, and the build will proceed. This
+can be used as a drop-in replacement for git push to deploy.`;


Have you pulled this out just because indentation with backticks is messy? There's a nicer solution to that: stripIndent (e.g. https://github.com/resin-io/resin-cli/blob/b7214a306c6b0a5e45387f93c9142219ff6d7b60/lib/actions/api-key.ts)

import { stripIndent } from 'common-tags'; ... export const push: CommandDefinition<...> = { help: stripIndent` This command can be used to start a build on the remote resin.io cloud builders. The given source directory will be added to a tarfile, which will be sent to the resin.io builder, and the build will proceed. This can be used as a drop-in replacement for git push to deploy. `,

pimterry · 2018-05-09T12:27:11Z

lib/actions/push.ts

+const PUSH_HELP_TEXT =
+`This command can be used to start a build on the remote resin.io cloud
+builders. The given source directory will be added to a tarfile, which
+will be sent to the resin.io builder, and the build will proceed. This


I'd skip the tarfile bit - I think a few users won't be familiar with tar files (especially on Windows etc), and it's not strictly necessary. Just will be send to the resin.io builder in a compressed form is fine imo.

pimterry · 2018-05-09T12:28:38Z

lib/actions/push.ts

+		exitWithExpectedError(
+			`No applications found with name: ${appName}.\n\n` +
+			'This could mean that the application does not exist, or you do \n' +
+			'not have the permissions required to access it.'


You probably want stripIndent here and a multiline string here too.

pimterry · 2018-05-09T12:30:44Z

lib/actions/push.ts

+`This command can be used to start a build on the remote resin.io cloud
+builders. The given source directory will be added to a tarfile, which
+will be sent to the resin.io builder, and the build will proceed. This
+can be used as a drop-in replacement for git push to deploy.`;


We also normally include an example command in the help text. This is what becomes the CLI docs on https://docs.resin.io/reference/cli/, and they get nicely rendered there (and they're useful here too).

https://github.com/resin-io/resin-cli/blob/b7214a306c6b0a5e45387f93c9142219ff6d7b60/lib/actions/device.coffee#L133-L143 is a nice example imo.

pimterry · 2018-05-09T12:32:50Z

lib/utils/remote-build.ts

+		stream.on('data', (data: BuilderMessage) => {
+			if (!spinnerStopped) {
+				uploadSpinner.stop();
+				spinnerStopped = true;


This spinner actually has its own check to make stop() safe: https://github.com/resin-io-modules/resin-cli-visuals/blob/edeb543a5c83cb67b684564f7835e8a69dfeaf6a/lib/widgets/spinner.coffee#L78

You could drop spinnerStopped entirely, and just call uploadSpinner.stop() every time.

pimterry · 2018-05-09T12:35:06Z

lib/utils/remote-build.ts

+				// FIXME: Make this error nicer.
+				console.log(
+					'Warning: ignoring unknown builder command. You may experience odd build output. ' +
+						'Maybe you need to update resin-cli?',


stripIndent

pimterry · 2018-05-09T12:53:43Z

lib/utils/remote-build.ts

+	const uploadSpinner = new visuals.Spinner('Uploading source package to resin cloud');
+	uploadSpinner.start();
+
+	tarStream.pipe(post);


In fact, imo we should add a data handler here that's tied to uploadSpinner.stop(), and not return the spinner at all. I think that should work fine, and lets the code everywhere else ignore the spinners entirely.

pimterry · 2018-05-09T16:11:49Z

lib/actions/push.ts

+> = {
+	signature: 'push <application>',
+	description: 'Start a remote build on the resin.io cloud build servers',
+	help: stripIndent`This command can be used to start a build on the remote


Sorry, if you test this help text, you'll see this doesn't work properly.

stripIndent strips the common indent, ignoring empty lines, so you'll need to start with an empty line. If you don't, as here, then the common indent is zero (the first line has none) and text all goes wrong.

Same for the other cases here too. You want things like:

help: stripIndent` This command can be used to start a build on the remote resin.io cloud builders. The given source directory will be sent to the resin.io builder, and the build will proceed. This can be used as a drop-in replacement for git push to deploy. Examples: $ resin push myApp $ resin push myApp --source <source directory> $ resin push myApp -s <source directory> `,

That should do what you expect.

Change-type: minor Connects-to: #843

ghost · 2018-05-10T12:09:59Z

@CameronDiver, status checks have failed for this PR. Please make appropriate changes and recommit.

CameronDiver requested review from dfunckt, zvin, thgreasi and pimterry April 25, 2018 18:09

CameronDiver force-pushed the 843-resin-push branch from 8a2dfde to 5cd1910 Compare April 25, 2018 18:58

balena-io deleted a comment Apr 25, 2018

CameronDiver force-pushed the 843-resin-push branch from 5cd1910 to 1a42340 Compare April 27, 2018 15:33

balena-io deleted a comment Apr 30, 2018

CameronDiver force-pushed the 843-resin-push branch from 1a42340 to ff6911a Compare April 30, 2018 15:31

pimterry suggested changes May 7, 2018

View reviewed changes

CameronDiver force-pushed the 843-resin-push branch 2 times, most recently from c9cb547 to 3e29cbe Compare May 9, 2018 10:46

CameronDiver requested a review from pimterry May 9, 2018 10:46

pimterry suggested changes May 9, 2018

View reviewed changes

CameronDiver force-pushed the 843-resin-push branch from 3e29cbe to 35aa8eb Compare May 9, 2018 15:25

CameronDiver requested a review from pimterry May 9, 2018 15:26

pimterry suggested changes May 9, 2018

View reviewed changes

CameronDiver force-pushed the 843-resin-push branch from 35aa8eb to 78b5b9a Compare May 10, 2018 10:25

CameronDiver requested a review from pimterry May 10, 2018 10:26

Add push command which starts a build on remote resin servers

439d8d3

Change-type: minor Connects-to: #843

CameronDiver force-pushed the 843-resin-push branch from 78b5b9a to 439d8d3 Compare May 10, 2018 10:43

pimterry approved these changes May 10, 2018

View reviewed changes

CameronDiver added the procbots/versionbot/ready-to-merge label May 10, 2018

CameronDiver removed the procbots/versionbot/ready-to-merge label May 10, 2018

CameronDiver force-pushed the 843-resin-push branch from 20b2d74 to 439d8d3 Compare May 10, 2018 12:12

CameronDiver added the procbots/versionbot/ready-to-merge label May 10, 2018

v7.4.0

c88dd22

ghost merged commit 22e7498 into master May 10, 2018

ghost removed the procbots/versionbot/ready-to-merge label May 10, 2018

ghost deleted the 843-resin-push branch May 10, 2018 12:44

CameronDiver mentioned this pull request May 28, 2018

The CLI should be able to trigger builds on resin's build server infrastructure #843

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add push command which starts a build on remote resin servers #868

Add push command which starts a build on remote resin servers #868

CameronDiver commented Apr 25, 2018 •

edited by ghost

Loading

pimterry May 3, 2018

pimterry May 7, 2018

pimterry May 7, 2018

pimterry May 7, 2018

pimterry May 7, 2018

pimterry May 7, 2018

CameronDiver May 9, 2018

pimterry May 7, 2018

CameronDiver May 9, 2018

pimterry May 7, 2018

CameronDiver May 9, 2018

pimterry May 7, 2018

pimterry May 7, 2018

ghost commented May 9, 2018

pimterry left a comment

pimterry May 9, 2018

pimterry May 9, 2018

pimterry May 9, 2018

pimterry May 9, 2018

pimterry May 9, 2018

pimterry May 9, 2018

pimterry May 9, 2018

pimterry May 9, 2018

ghost commented May 10, 2018


		import { tarDirectory } from './compose';

		const DEBUG_MODE = process.env.DEBUG === '1';

Add push command which starts a build on remote resin servers #868

Add push command which starts a build on remote resin servers #868

Conversation

CameronDiver commented Apr 25, 2018 • edited by ghost 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

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

ghost commented May 9, 2018

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

Choose a reason for hiding this comment

ghost commented May 10, 2018

CameronDiver commented Apr 25, 2018 •

edited by ghost

Loading