Auto-generate updates for the deploy repository using Docker #30

d00rman · 2015-04-17T17:43:40Z

This PR is a port of wikimedia/service-template-node#36 to service-runner. It holds the same functionality, but with different command-line arguments:

-s, --docker-start - starts the service inside a container
-t, --docker-test - starts the test process inside a container
-b, --build - builds/updates the deploy repository
-f, --force-build - forces node module rebuild even if there are no changes in the source repository
-r, --review - build the deploy repository and send the patch for review to Gerrit

Additionally, a small fix has been included that shows the service's version (and not that of service-runner) when the -v command-line argument is supplied.

Bug: T96128

Docker container

service-runner

d00rman · 2015-04-17T19:07:30Z

cc @gwicke @eevans

gwicke · 2015-04-17T20:27:36Z

This PR is a port of wikimedia/service-template-node#36 to service-runner. It holds the same functionality, but with different command-line arguments:

-s, --docker-start - starts the service inside a container
-t, --docker-test - starts the test process inside a container

How about start-docker and test-docker as positional action parameters, along with npm start-docker and npm test-docker?

-b, --build - builds/updates the deploy repository

Since we want to build other things too, how about build deploy-repo?

-f, --force-build - forces node module rebuild even if there are no changes in the source repository

This could be documented as more orthogonal --force, with what is forced depending on the main action.

-r, --review - build the deploy repository and send the patch for review to Gerrit

This is very WMF-specific. Can we document this as an option to the main action (build deploy-repo in this case)?

gwicke · 2015-04-17T20:29:41Z

lib/docker.js

+ * @param {Boolean} capture whether to capture stdout and return its contents
+ * @return {Promise} the promise which is fulfilled once the child exists
+ */
+function promised_spawn(args, capture) {


You could use execFileAsync (promisified child_proces.execFile) instead.

For some executions I want to capture the output altogether (like git status), while for others it is good for the user to see the output in real time (like building the image, or npm install).

gwicke · 2015-04-17T21:12:11Z

LGTM in principle, just some detail stuff / naming ideas.

Docker-related operations (build, start, test) are now commands, with 'build' having optional '--deploy-repo' and '--review' parameters. Also, '--force' and '--verbose' have been added as general-puropose args. The output of help has also been improved.

d00rman · 2015-04-20T10:19:14Z

How about start-docker and test-docker as positional action parameters, along with npm start-docker and npm test-docker?
-b, --build - builds/updates the deploy repository
Since we want to build other things too, how about build deploy-repo?

-f, --force-build - forces node module rebuild even if there are no changes in the source repository
This could be documented as more orthogonal --force, with what is forced depending on the main action.

-r, --review - build the deploy repository and send the patch for review to Gerrit
This is very WMF-specific. Can we document this as an option to the main action (build deploy-repo in this case)?

All of these have been addressed in 971fe0e . I put docker-start and docker-test instead, since start-docker and test-docker imply (to me, at least) you want to start and test Docker, whereas docker-start and docker-test somewhat clarify that you want Docker to start and test your service.

All of its options are reachable from service-runner's start script, so these are no longer needed.

d00rman · 2015-04-20T10:22:39Z

along with npm start-docker and npm test-docker?

I didn't put this one, as I can see its use. Defining npm scripts in package.json makes them reachable only from service-runner's package, not its dependants. The service template already has these, I'll just modify them to use service-runner instead of docker.js once this PR is merged, and the new version ihas been published.

d00rman · 2015-04-20T10:51:43Z

'debian'? ;)

Should we use https://registry.hub.docker.com/_/debian/ ? It is recommended on the Debian wiki. They also have a review of the differences between the image and a vanilla debootstrap that sounds reasonable.

Done in 4e6adde

d00rman · 2015-04-20T14:54:26Z

Can we make 'build' more universal? We'll want to build other things too, like debian packages, docker images, init scripts etc. build deploy-repo would leave the door open for that.

Not sure I follow. Currently, service-runner build is intended (reserved) for building everything, while service-runner build --deploy-repo builds only the deploy repo.

d00rman · 2015-04-20T15:29:01Z

Since the builds are mutually exclusive and have different options,

TBH, I'm still in a pickle whether they should be mutually-exclusive. If so, then I'd be fine with build-deploy[-repo] and build-package as well. If, on the other hand, build alone builds everything there is to build, then options would be more appropriate wrt consistency IMO.

I still think that subcommands would be cleaner and easier to document.

Heh, I'm not sure that's true, given that yargs doesn't support subcommands, so having them would involve having more code based on the chosen command (parser resetting, adding options etc), not to mention the fact that we couldn't profit from its autogenerated docs. I would be in favour of passing totally to commands, as the current dual system seems a bit inconsistent, e.g. -c doesn't need a command, while -r does. Maybe if we introduced a run command and make it the default? (We would still need to build help docs ourselves in that case)

Granted, the current help message could be further enriched and bettered, but it's already a step forward :)

gwicke · 2015-04-20T15:46:39Z

lib/docker.js

+                '-regextype',
+                'posix-egrep',
+                '-iregex',
+                "(.*\\.git.*|.*\\.md|.*\\.txt|.*readme|.*licence)",


Afaik the only things we really need to remove are .gitignore files, as they'll mess with the checkin. Removing the other files you match here is probably fine, but I'm not sure that it's worth it.

Auto-generate updates for the deploy repository using Docker

gwicke · 2015-04-20T19:37:01Z

Merged as-is per discussion this morning. Plan is to beautify argument / command handling in a follow-up.

d00rman added 5 commits April 17, 2015 16:14

Add the docker.js script allowing to run/build the service inside of a

8cbef4c

Docker container

Minor fix: Display the version of the service, not that of

6cc750b

service-runner

Bump version to 0.1.8

070382e

Minor: remove child error console.log()

6d603d3

Let the user choose whether to git review or not (default: false)

be53b1e

d00rman mentioned this pull request Apr 17, 2015

Auto-generate updates for the deploy repository wikimedia/service-template-node#36

Closed

gwicke reviewed Apr 17, 2015
View reviewed changes

d00rman added 2 commits April 20, 2015 12:10

Minor: Set the deployRepo arg if review is set

1c54873

Remove command-line args from lib/docker.js

2fc0f3d

All of its options are reachable from service-runner's start script, so these are no longer needed.

Use the official debian image and set it as the default target

4e6adde

gwicke reviewed Apr 20, 2015
View reviewed changes

gwicke added a commit that referenced this pull request Apr 20, 2015

Merge pull request #30 from d00rman/docker

95d04d3

Auto-generate updates for the deploy repository using Docker

gwicke merged commit 95d04d3 into wikimedia:master Apr 20, 2015

d00rman deleted the docker branch July 6, 2015 21:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auto-generate updates for the deploy repository using Docker #30

Auto-generate updates for the deploy repository using Docker #30

d00rman commented Apr 17, 2015

d00rman commented Apr 17, 2015

gwicke commented Apr 17, 2015

gwicke Apr 17, 2015

d00rman Apr 20, 2015

gwicke commented Apr 17, 2015

d00rman commented Apr 20, 2015

d00rman commented Apr 20, 2015

d00rman commented Apr 20, 2015

d00rman commented Apr 20, 2015

d00rman commented Apr 20, 2015

gwicke Apr 20, 2015

gwicke commented Apr 20, 2015

Auto-generate updates for the deploy repository using Docker #30

Auto-generate updates for the deploy repository using Docker #30

Conversation

d00rman commented Apr 17, 2015

d00rman commented Apr 17, 2015

gwicke commented Apr 17, 2015

gwicke Apr 17, 2015

Choose a reason for hiding this comment

d00rman Apr 20, 2015

Choose a reason for hiding this comment

gwicke commented Apr 17, 2015

d00rman commented Apr 20, 2015

d00rman commented Apr 20, 2015

d00rman commented Apr 20, 2015

d00rman commented Apr 20, 2015

d00rman commented Apr 20, 2015

gwicke Apr 20, 2015

Choose a reason for hiding this comment

gwicke commented Apr 20, 2015