-
Notifications
You must be signed in to change notification settings - Fork 176
Conversation
…name. Rendering a .dockerapp dir is not allowed anymore Signed-off-by: Jean-Christophe Sirot <[email protected]>
bce6701
to
c638020
Compare
internal/commands/render.go
Outdated
if err != nil { | ||
if strings.HasSuffix(appname, ".dockerapp") { | ||
return nil, nil, nil, fmt.Errorf("%q looks like a docker App directory and App must be built first before rendering", appname) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really want to be this specific? If we do, then why not print out the command the user needs to run to build the app?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could do the same as in docker app image tag who throw this type of error: "could not tag %q: no such App image". So here something like: "could not render %q: no such App image"?
internal/commands/render.go
Outdated
if err != nil { | ||
if strings.HasSuffix(appname, ".dockerapp") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this test should be moved to cnab.GetBundle
? So all the commands would have the same behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should just remove the check for .dockerapp
? What if I want to name my application toto.dockerapp
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If your application is toto.dockerapp
the GetBundle should return a bundle and triggers no error but I'm also wondering if having that specific warning for .dockerapp
is useful
Signed-off-by: Jean-Christophe Sirot <[email protected]>
c638020
to
7263c18
Compare
Codecov Report
@@ Coverage Diff @@
## master #724 +/- ##
==========================================
+ Coverage 71.49% 71.82% +0.32%
==========================================
Files 59 59
Lines 3280 3329 +49
==========================================
+ Hits 2345 2391 +46
- Misses 605 606 +1
- Partials 330 332 +2
Continue to review full report at Codecov.
|
Signed-off-by: Jean-Christophe Sirot <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
- What I did
This PR ensure that the
docker app render
takes a App image reference argument and fails when a.dockerapp
directory is used. If the image is not present locally in the bundle store, the App image is automatically fetched.- How to verify it
Run
docker app render myapp.dockerapp
and verify it fails with an error indicating that App must be built first.Run
docker app render myapp:mytag --set param=some_value
and verify the app is correctly rendered.- A picture of a cute animal (not mandatory but encouraged)
