Add --path argument to container:push #75
Conversation
Codecov Report
@@ Coverage Diff @@
## master #75 +/- ##
=======================================
Coverage 87.45% 87.45%
=======================================
Files 12 12
Lines 287 287
=======================================
Hits 251 251
Misses 36 36
Continue to review full report at Codecov.
|
lib/sanbashi.js
Outdated
| let cwd = Path.dirname(dockerfile) | ||
| if (path) { | ||
| cwd = Path.join(process.cwd(), path) |
There was a problem hiding this comment.
Sanbashi shouldn't use process.cwd(). If we need an absolute path here, it should be passed upstream.
Also, what if I do pass an absolute path to the command?
There was a problem hiding this comment.
I don't think an absolute path is needed here. I'll just pass whatever comes on --path as an arg to docker build
commands/push.js
Outdated
| @@ -15,7 +15,8 @@ module.exports = function (topic) { | |||
| `${cli.color.cmd('heroku container:push worker')} # Pushes Dockerfile to worker process type`, | |||
| `${cli.color.cmd('heroku container:push web worker --recursive')} # Pushes Dockerfile.web and Dockerfile.worker`, | |||
| `${cli.color.cmd('heroku container:push --recursive')} # Pushes Dockerfile.*`, | |||
| `${cli.color.cmd('heroku container:push web --arg ENV=live,HTTPS=on')} # Build-time variables` | |||
| `${cli.color.cmd('heroku container:push web --arg ENV=live,HTTPS=on')} # Build-time variables`, | |||
| `${cli.color.cmd('heroku container:push --recursive --path .')} # Pushes Dockerfile.* using current dir as build context` | |||
There was a problem hiding this comment.
Not sure we should give an example for every option. @heroku/cli wdyt?
commands/push.js
Outdated
| @@ -33,6 +34,12 @@ module.exports = function (topic) { | |||
| name: 'arg', | |||
| hasValue: true, | |||
| description: 'set build-time variables' | |||
| }, | |||
| { | |||
| name: 'path', | |||
There was a problem hiding this comment.
--path is a bit obscure IMHO. I'd name this --context, or maybe even --context-path.
There was a problem hiding this comment.
Agreed, will change to --context-path
|
Could you add a test for the command as well? |
Fixes #74