Exporter defaults to stack.toml run image
#127
Conversation
cmd/exporter/main.go
Outdated
| if flag.NArg() > 1 || flag.Arg(0) == "" || runImageRef == "" { | ||
| args := map[string]interface{}{"narg": flag.NArg(), "runImage": runImageRef, "layersDir": layersDir} | ||
| if flag.NArg() > 1 || flag.Arg(0) == "" { | ||
| args := map[string]interface{}{"narg": flag.NArg(), "layersDir": layersDir} |
There was a problem hiding this comment.
To be fair this error was already weird before this PR. It feels like we should print all of the optional args in this error message or none of them. Just including the layersDir is strange. Providing none of them would be consistent with the builder and detector
cmd/exporter/main.go
Outdated
| var stack metadata.StackMetadata | ||
| _, err = toml.DecodeFile(stackPath, &stack) | ||
| if err != nil { | ||
| outLog.Printf("no stack.toml found at path '%s', stack metadata will not be exported\n", stackPath) | ||
| } | ||
|
|
||
| if runImageRef == "" { | ||
| if stack.RunImage.Image == "" { | ||
| return cmd.FailCode(cmd.CodeInvalidArgs, "--run-image is required when stack.toml is not present in builder") |
There was a problem hiding this comment.
The lifecycle flag is flag is -image rather than --run-image. Given that the stack.toml can have an arbitrary name if the path is supplied with a flag or an environment variable, perhaps we should say something more generic like "-image is required when there is no stack metadata available". Also, I'm not sure we should mention builders here, that feels like a platform specific concept.
cmd/exporter/main.go
Outdated
| if stack.RunImage.Image == "" { | ||
| return cmd.FailCode(cmd.CodeInvalidArgs, "--run-image is required when stack.toml is not present in builder") | ||
| } | ||
| runImageRef = stack.RunImage.Image |
There was a problem hiding this comment.
We want to select the best mirror (the one whose registry matches repoName) instead of always using stack.RunImage.Image. stack.RunImage.Image can be the default if none of the mirrors have a matching registry.
joaopapereira
force-pushed
the
issue-119
branch
from
e3f3015 to
d764eca
Compare
Remove the required `--image` command line argument and default to `stack.toml` configuration. When the argument is passed it overrides the `stack.toml` value Signed-off-by: Joao Pereira <[email protected]>
Remove unnecessary information Signed-off-by: Joao Pereira <[email protected]>
joaopapereira
force-pushed
the
issue-119
branch
from
d764eca to
8ab12cc
Compare
|
@ekcasey I added the functionality of choosing the Run Image by registry inside the image package. But only after doing so I realize that it was split to a separated repo. This feels like a functionality that should be in imageUtil, do you think I should create it in there and when it is merge have update this PR? |
|
@joaopapereira given that the only thing in imgutil right now is our image abstraction, I think this functionality fits better in the lifecycle repo |
thisisnotashwin
force-pushed
the
issue-119
branch
from
8ab12cc to
9574613
Compare
image/image.go
Outdated
| continue | ||
| } | ||
| if registry == reg { | ||
| return i, nil |
There was a problem hiding this comment.
This is a minor thing but I was confused the first time I glanced over this code b/c the variable name i led me to assume it was referring to the array index.
cmd/exporter/main.go
Outdated
| "io/ioutil" | ||
| "log" | ||
| "os" | ||
| "path/filepath" | ||
|
|
||
| "github.com/BurntSushi/toml" | ||
| "github.com/buildpack/imgutil" | ||
| "github.com/buildpack/lifecycle/image" |
There was a problem hiding this comment.
we should group this with the other imports from lifecycle
Enhance the selection of run image by using the mirror that is in the same registry or the canonic version if no mirror is colocated Signed-off-by: Joao Pereira <[email protected]>
thisisnotashwin
force-pushed
the
issue-119
branch
from
9574613 to
14263b3
Compare
|
@ekcasey done 😺 |
Remove the required
--imagecommand line argument and defaultto
stack.tomlconfiguration.When the argument is passed it overrides the
stack.tomlvalueFixes #119