Multiple dockerfiles #6

hunterloftis · 2016-10-04T22:57:48Z

Okay @tt and @jbyrum, got a preliminary implementation of the multiple dockerfiles UX. To test:

pull this branch
npm install
heroku plugins:link
push @jbyrum's test apps (https://github.com/jbyrum/multidockerfiles-1, https://github.com/jbyrum/multidockerfiles-2)

Should go something like:

v6.2.0:~/test/multidockerfiles-1 (master)$ heroku container:push
 ▸    Using nearest match for the 'web' process:
 ▸    /Users/hloftis/test/multidockerfiles-1/Dockerfile (used)
 ▸    /Users/hloftis/test/multidockerfiles-1/webapp/Dockerfile.web
Building web (/Users/hloftis/test/multidockerfiles-1/Dockerfile)
Sending build context to Docker daemon 135.2 kB
Step 1 : FROM alpine:latest
 ---> ee4603260daa
Step 2 : RUN apk add --update python py-pip bash
 ---> Using cache
 ---> 3af33f3fbfa2
Step 3 : ADD ./webapp/requirements.txt /tmp/requirements.txt
 ---> Using cache
 ---> b054ad539efe
Step 4 : RUN pip install -qr /tmp/requirements.txt
 ---> Using cache
 ---> 47810befab5f
Step 5 : ADD ./webapp /opt/webapp/
 ---> Using cache
 ---> 656348035406
Step 6 : WORKDIR /opt/webapp
 ---> Using cache
 ---> aba264f7e49d
Step 7 : CMD python app.py
 ---> Using cache
 ---> f056bba8f77f
Successfully built f056bba8f77f
Building image (/Users/hloftis/test/multidockerfiles-1/image-processing/Dockerfile.image)
Sending build context to Docker daemon 3.072 kB
Step 1 : FROM alpine:latest
 ---> ee4603260daa
Step 2 : RUN apk add --update python py-pip bash
 ---> Using cache
 ---> 3af33f3fbfa2
Step 3 : ADD . /opt/image-processing/
 ---> Using cache
 ---> 6edebd0cda1d
Step 4 : WORKDIR /opt/image-processing
 ---> Using cache
 ---> 0b000098e489
Step 5 : CMD python image-processing.py
 ---> Using cache
 ---> 7aca76942497
Successfully built 7aca76942497
Building worker (/Users/hloftis/test/multidockerfiles-1/worker/Dockerfile.worker)
Sending build context to Docker daemon 3.072 kB
Step 1 : FROM alpine:latest
 ---> ee4603260daa
Step 2 : RUN apk add --update python py-pip bash
 ---> Using cache
 ---> 3af33f3fbfa2
Step 3 : ADD . /opt/worker/
 ---> Using cache
 ---> 69dfae0635ad
Step 4 : WORKDIR /opt/worker
 ---> Using cache
 ---> cae090ddea7e
Step 5 : CMD python worker.py
 ---> Using cache
 ---> 565ef506cb42
Successfully built 565ef506cb42
Pushing 3 images...
Pushing web (/Users/hloftis/test/multidockerfiles-1/Dockerfile)
The push refers to a repository [registry.heroku.com/whispering-beyond-11493/web]
ba6984bf4df2: Layer already exists
62affd5f66a7: Layer already exists
be752c7999c0: Layer already exists
e577649c1afa: Layer already exists
9007f5987db3: Layer already exists
latest: digest: sha256:72bf59ac080eca6f22e4a38e7b67f6a6df1af71fce065a537749a2d5d7147773 size: 1366
Pushing image (/Users/hloftis/test/multidockerfiles-1/image-processing/Dockerfile.image)
The push refers to a repository [registry.heroku.com/whispering-beyond-11493/image]
2504e8086f59: Layer already exists
e577649c1afa: Layer already exists
9007f5987db3: Layer already exists
latest: digest: sha256:bb726cb810be83c542877c940d4109c90f7e2f16cdd1bf44671cdc1fda1e47df size: 947
Pushing worker (/Users/hloftis/test/multidockerfiles-1/worker/Dockerfile.worker)
The push refers to a repository [registry.heroku.com/whispering-beyond-11493/worker]
3d3bf091d8a8: Layer already exists
e577649c1afa: Layer already exists
9007f5987db3: Layer already exists
latest: digest: sha256:89f2bef3c07ef8ca1ab155de03ce3d08db11448dff7f107ce3dc5d6a1bcb0447 size: 947

…us Dockerfile matches

* More explicit error messages

use inquirer to pick

jbyrum · 2017-05-03T18:16:04Z

@KarateCowboy -- Does this need to be merged before I can play with it?

cc @tt

jbyrum · 2017-05-03T18:16:14Z

@KarateCowboy -- Does this need to be merged before I can play with it?

cc @tt

jdx · 2017-05-03T18:25:06Z

you can run the plugins locally by checking out the code and running heroku plugins:link .

KarateCowboy · 2017-05-03T18:25:33Z

@jbyrum no just check out the multiple-dockerfiles branch, then do as @dickeyxxx just suggested with heroku plugins:link

jdx · 2017-05-03T18:51:09Z

multiple stuff seems good, I was able to get inquirer to at least prompt but it would probably be good to have @jbyrum do a more thorough test

jdx · 2017-05-03T18:53:37Z

test/sanbashi.test.js

+      expect(results.web[1]).to.have.property('depth', 2, 'dockerfile', './Nested/Dockerfile.web', 'postfix', 1)
+    })
+    it('filters out by process type', () => {
+      const dockerfiles = ['./Dockerfile.web', './Nested/Dockerfile.worker']


these should use path.join() so they could work on windows

jdx · 2017-05-03T18:54:11Z

commands/push.js


  try {
-    let build = yield buildImage(resource, context.cwd, context.flags.verbose);
+    for (let job of jobs) {
+      cli.log(`\n=== Building ${ job.name } (${ job.dockerfile })`)


cli.header might be preferable

jdx · 2017-05-03T18:55:59Z

commands/push.js

+  let herokuHost = process.env.HEROKU_HOST || 'heroku.com'
+  let registry = `registry.${ herokuHost }`
+  let dockerfiles = Sanbashi.getDockerfiles(process.cwd(), recurse)
+  let possibleJobs = Sanbashi.getJobs(`${ registry }/${ context.app }`, context.args, dockerfiles)


path.join here

Is this a path?

whoops, no it isn't

So we should concat manually?

Yeah this code is fine

jdx · 2017-05-03T18:56:29Z

commands/push.js

+  let jobs = yield Sanbashi.chooseJobs(possibleJobs)
+  if (!jobs.length) {
+    cli.warn('No images to push')
+    process.exit()


this is an error to me not a place for a warning

hunterloftis · 2017-05-03T20:17:10Z

If the CLI uses the latest version of node now there's a good opportunity to use modern async constructs as well (like async/await instead of co).

jdx · 2017-05-05T17:09:10Z

package.json

    "is-there": "4.0.0",
    "lodash": "3.3.1",
    "mkdirp": "^0.5.0",
-    "node-uuid": "^1.4.3",
+    "node-uuid": "1.4.3",


this package is deprecated

jdx · 2017-05-05T17:09:38Z

package.json

@@ -19,28 +19,29 @@
  "license": "ISC",
  "dependencies": {
    "camelcase": "1.0.2",
+    "chalk": "1.1.3",


use cli.color not chalk directly

jdx · 2017-05-05T17:11:22Z

commands/push.js

+  if (context.args.length === 0 && !recurse) {
+    cli.error(`Error: Requires either --recursive or one or more process types
+		${Chalk.bold.underline.magenta('Usage:')}
+			${ Chalk.white('  heroku container:push web')}         	           # Pushes Dockerfile in current directory


we have a style for this: cli.color.cmd() check out the style guide

use Path.join some formatting changes

Jbyrum text updates

jdx · 2017-05-23T17:36:48Z

commands/login.js

  }
  catch (err) {
-    cli.error(`Error: docker login exited with ${ err }`);
+    cli.error(`Error: docker login exited with ${ err }`)
+    cli.hush(err.stack || err)


this should exit with a nonzero code, it looks like it's exiting with 0

jdx · 2017-05-23T17:38:14Z

commands/push.js

-const child = require('child_process');
+let usage = `
+  ${cli.color.bold.underline.magenta('Usage:')}
+  ${ cli.color.white('heroku container:push web')}         	           # Pushes Dockerfile in current directory to web process type


use cli.color.cmd

jdx · 2017-05-23T17:40:33Z

commands/push.js

  }

  try {
-    let push = yield pushImage(resource, context.flags.verbose);
+    for (let job of jobs) {
+     cli.styledHeader(`Pushing  ${job.name}  (${job.dockerfile })`)


there are a lot of little style issues like the indentation on this line. It's probably too much work to put it in for the whole project, but it would be good to run it at least on the new code

jdx · 2017-05-23T17:41:33Z

commands/logout.js


 module.exports = function(topic) {
  return {
    topic: topic,
    command: 'logout',
    flags: [{ name: 'verbose', char: 'v', hasValue: false }],
-    description: 'Logs out from the Heroku Docker registry',
+    description: 'logs out from the Heroku Container Registry',
+    help: `Usage:


the CLI already automatically adds a "usage" help bit. Just take the help attribute out here since it's not adding anything useful

jbyrum

I ran heroku container:push web -a alpine-helloworld-dogwood

The header said "=== Building standard" rather than "=== Building web"

It is also pushing to a process type called 'standard':
v50 Deployed standard (701dbd3a2818) [email protected] 2017/05/23 15:31:20 -0700 (~ 7s ago)

KarateCowboy · 2017-06-05T21:39:35Z

@jbyrum fixed that issue

jbyrum · 2017-06-12T20:49:30Z

This is now working for me -- I don't see any additional bugs.

What's next? @dickeyxxx @tt

Code updated. please look again

KarateCowboy · 2017-06-15T18:57:42Z

@dickeyxxx poke poke. Please review

jdx · 2017-06-15T19:03:01Z

commands/login.js

+    flags: [{name: 'verbose', char: 'v', hasValue: false}],
+    description: 'logs in to the Heroku Container Registry',
+    help: `Usage:
+        heroku container:login`,


missing the $ sign and this example shold show some output

jdx · 2017-06-15T19:03:21Z

commands/login.js

-    flags: [{ name: 'verbose', char: 'v', hasValue: false }],
-    description: 'Logs in to the Heroku Docker registry',
+    flags: [{name: 'verbose', char: 'v', hasValue: false}],
+    description: 'logs in to the Heroku Container Registry',


"logs into"

"logs in to" is actually correct. "in" is part of the verb phase

so I recently ran through our entire help and audited everything: heroku/cli-engine#196

What I found was there is a mix of using present tense and another type that I think might be "imperative mood" but I'm not sure. Anyways, this "imperative mood" (if that is the correct term) is used much more commonly and I think makes more sense.

So in this case it would be "log in to the Heroku Container Registry". Not "logs in to the Heroku Container Registry". I plan to add this to the style guide.

Also, I'm not sure what language we use elsewhere in documentation, but I'm pretty sure in the CLI we never use "log in" as 2 words, it's always "login" and "logout". So I think my first comment here was wrong for a few reasons.

jdx · 2017-06-15T19:14:06Z

commands/push.js


+  let possibleJobs = Sanbashi.getJobs(`${ registry }/${ context.app }`, dockerfiles)
+  let jobs = []
+  if(recurse){


can you run standard on this file at least to fix the formatting?

jdx · 2017-06-15T19:18:06Z

commands/login.js

-    child.spawn('docker', args, { stdio: 'inherit' })
+    ]
+    log(verbose, args)
+    child.spawn('docker', args, {stdio: 'inherit'})


we should abstract this into 1 spawn function that returns a promise

jdx · 2017-06-15T19:21:16Z

lib/sanbashi.js

+}
+
+Sanbashi.pushImage = function (resource, verbose) {
+  console.dir(resource, { colors:true, depth: null })


is this supposed to be in here?

jdx · 2017-06-15T19:40:28Z

commands/push.js

+let usage = `
+    ${cli.color.bold.underline.magenta('Usage:')}
+    ${ cli.color.cmd('heroku container:push web')}         	           # Pushes Dockerfile in current directory to web process type
+    ${ cli.color.cmd('heroku container:push web worker --recursive')}     # Pushes Dockerfile.web and Dockerfile.worker found in current & subdirectories


these lines are far too long

should show output too if possible

Thanks for the feedback -- @KarateCowboy, can we update with this text? It adds new help text to -R and new text for the heroku container:push examples

https://docs.google.com/document/d/1DbuvumF3aerFqO-1h0W0evuVLFmV9XOp5bEXEnvSi3Y/edit#heading=h.kme0kogsywhu

soon(-ish) we plan to have a format for examples since we use them so much: heroku/cli-engine#190

jdx · 2017-06-15T19:41:18Z

commands/logout.js

-    flags: [{ name: 'verbose', char: 'v', hasValue: false }],
-    description: 'Logs out from the Heroku Docker registry',
+    flags: [{name: 'verbose', char: 'v', hasValue: false}],
+    description: 'logs out from the Heroku Container Registry',
    needsApp: false,


can you add help with an output example?

jdx · 2017-06-15T19:41:33Z

commands/index.js

  return {
    topic: pkg.topic,
    description: pkg.description,
-    help: pkg.description,


can you put the help back in with an output example?

To which command are you referring? the basic heroku containers just prints the version. What would you like added -- the sum of all the help for the other commands?

an example of it printing the version then, we're supposed to have output examples for every command. I realize that isn't the most useful thing in the world, but it clearly demonstrates what a command is for.

this is the (soon-to-be enforced) rule for core plugins anyways, but we should do it on any plugin we can

Made text updates to push

the same code was repeated in several places to spawn docker commands. Consolidated this into a helper method

changes addressed please review again

hunterloftis added 2 commits October 4, 2016 19:03

4.1.1

4d70559

push multiple, recursive, choose nearest, warn multiple, fail ambiguo…

d7cf202

…us Dockerfile matches

hunterloftis force-pushed the multiple-dockerfiles branch from 44b2c46 to d7cf202 Compare October 4, 2016 23:04

jbyrum and others added 4 commits October 5, 2016 11:07

Update push.js

e66a79b

* More explicit error messages

more readable formatting

a66925e

multiple dockerfiles

502e3f1

use inquirer to pick

some changes for excluding certain files

b4c8b85

KarateCowboy requested review from jdx and jbyrum May 3, 2017 18:24

jdx reviewed May 3, 2017

View reviewed changes

jdx reviewed May 5, 2017

View reviewed changes

KarateCowboy force-pushed the multiple-dockerfiles branch from 0eedbe1 to 434c6c4 Compare May 5, 2017 19:55

review feedback

9522087

use Path.join some formatting changes

KarateCowboy force-pushed the multiple-dockerfiles branch from 434c6c4 to 9522087 Compare May 5, 2017 21:08

KarateCowboy and others added 8 commits May 11, 2017 16:32

minor fixes and updates

c00e780

remove heroku-client dependency

99f90f8

do not treat 'Dockerfile' as 'Dockerfile.web'

c940247

use regular dockerfile when recurse arg omitted

be01673

Remove docker text

be7651f

Remove Docker text

5b148f9

Update to usage text

fc6fc7b

Merge pull request #17 from heroku/jbyrum-text-updates

a3b4a22

Jbyrum text updates

change headers to use cli.styledHeader

632a3c8

jdx previously requested changes May 23, 2017

View reviewed changes

jbyrum reviewed May 23, 2017

View reviewed changes

KarateCowboy requested a review from jdx May 25, 2017 18:49

KarateCowboy added 2 commits June 5, 2017 16:28

review feedback

832be4e

fixed merge conflicts

db8f6db

KarateCowboy added 3 commits June 9, 2017 10:16

fix pushing standard dockerfiles

0773479

more regression fixing

4f8e25e

fix recursive regression

7377b6c

add handling for directories w/o dockerfiles

cbcf60e

jdx previously requested changes Jun 15, 2017

View reviewed changes

jdx reviewed Jun 15, 2017

View reviewed changes

jbyrum and others added 5 commits June 19, 2017 09:57

Made text updates to push

f18dae3

logout/in command fix

df8ce5f

login text change

d744280

Merge pull request #18 from heroku/new-push-text

fb84194

Made text updates to push

consolidate spawing methods into helper

ec54401

the same code was repeated in several places to spawn docker commands. Consolidated this into a helper method

jdx approved these changes Jun 30, 2017

View reviewed changes

KarateCowboy merged commit 36cb431 into master Jul 5, 2017

KarateCowboy deleted the multiple-dockerfiles branch July 5, 2017 21:39

Multiple dockerfiles #6

Multiple dockerfiles #6

Conversation

hunterloftis commented Oct 4, 2016 • edited Loading

jbyrum commented May 3, 2017

jbyrum commented May 3, 2017

jdx commented May 3, 2017

KarateCowboy commented May 3, 2017

jdx commented May 3, 2017

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

hunterloftis commented May 3, 2017

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

jbyrum left a comment • edited Loading

Choose a reason for hiding this comment

KarateCowboy commented Jun 5, 2017

jbyrum commented Jun 12, 2017 • edited Loading

KarateCowboy commented Jun 15, 2017

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

hunterloftis commented Oct 4, 2016 •

edited

Loading

jbyrum left a comment •

edited

Loading

jbyrum commented Jun 12, 2017 •

edited

Loading