Skip to content
This repository has been archived by the owner on Jun 13, 2021. It is now read-only.

render command renders App image only #724

Merged
merged 3 commits into from
Nov 5, 2019

Conversation

jcsirot
Copy link
Contributor

@jcsirot jcsirot commented Oct 31, 2019

- 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)
image

…name. Rendering a .dockerapp dir is not allowed anymore

Signed-off-by: Jean-Christophe Sirot <[email protected]>
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)
Copy link
Member

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?

Copy link
Contributor

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"?

if err != nil {
if strings.HasSuffix(appname, ".dockerapp") {
Copy link
Contributor

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?

Copy link
Member

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?

Copy link
Contributor Author

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]>
@codecov
Copy link

codecov bot commented Oct 31, 2019

Codecov Report

Merging #724 into master will increase coverage by 0.32%.
The diff coverage is 94.44%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
internal/names.go 100% <ø> (ø) ⬆️
internal/commands/run.go 70.54% <100%> (+0.46%) ⬆️
internal/commands/image/list.go 83.33% <100%> (ø) ⬆️
internal/packager/cnab.go 97.91% <100%> (+0.18%) ⬆️
internal/commands/parameters.go 86.51% <84.61%> (-0.33%) ⬇️
internal/commands/render.go 76.71% <0%> (+5.8%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cbf819c...c2f76a8. Read the comment docs.

Copy link
Contributor

@carolinebriaud carolinebriaud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@silvin-lubecki silvin-lubecki merged commit 6f5737a into docker:master Nov 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants