Add container:run command #29
Conversation
README.md
Outdated
| @@ -13,6 +13,7 @@ $ heroku plugins:install heroku-container-registry | |||
| In a directory with a Dockerfile: | |||
|
|
|||
| ``` | |||
| $ heroku container:run web | |||
There was a problem hiding this comment.
I would maybe keep the readme focused on the minimal thing to deploy.
commands/run.js
Outdated
| { | ||
| name: 'arg', | ||
| hasValue: true, | ||
| description: 'set build-time variables' |
There was a problem hiding this comment.
I think we should decouple these commands (and remove this flag). It's not great that all commands need to know about building images.
There was a problem hiding this comment.
You mean have a container:build command and not run here (nor in push) anymore?
There was a problem hiding this comment.
Yes. We probably need to keep it part of the push command for compatibility but I think we should separate it for new commands (and eventually for the push command too).
commands/run.js
Outdated
| jobs = possibleJobs.standard || [] | ||
| } | ||
| if (!jobs.length) { | ||
| cli.warn('No images to push') |
commands/run.js
Outdated
|
|
||
| let jobs = [] | ||
| if (possibleJobs.standard) { | ||
| possibleJobs.standard.forEach((pj) => { pj.resource = pj.resource.replace(/standard$/, processType)}) |
There was a problem hiding this comment.
I have no idea. Looks like it's an array method. I've taken this from the push command.
There was a problem hiding this comment.
I was curious about resource.replace(/standard$/, processType) specifically.
There was a problem hiding this comment.
Maybe it's copied from another place there too? Can we remove it?
There was a problem hiding this comment.
@heroku/cli any idea about this?
There was a problem hiding this comment.
no idea. I looked at the code and it seems that it never gets set from what I can tell.
There was a problem hiding this comment.
standard come from here.
The lib detects the process name part of Dockerfile.web, and defaults to standard if there is nothing.
Maybe this should be renamed to default to be clearer.
There was a problem hiding this comment.
Wouldn't "web" be better as that's the default process type?
There was a problem hiding this comment.
web works for me. I'll change it.
dmathieu
force-pushed
the
run
branch
2 times, most recently
from
6e3d043
to
d71fea3
Compare
We may want to change the syntax a bit.
Closes /issues/28