As a buildpack author, I must specify run images in my builder.toml

[sclevine]

Acceptance Criteria
When I run pack create-builder my-builder -b ./builder.toml
Then builder.toml must contain the following mandatory fields for the command to succeed:

[stack]
id = ""
build-image = ""
run-image = ""

And builder.toml may contain the following optional field:

[stack]
run-image-mirrors = [""]

Such That During pack build, run image mirrors are considered in the following order:

1. Locally-defined mirrors in [[run-images]] in config.toml (see replace configure-builder with configure-run-image #38)
2. Mirrors specified in the builder metadata
3. The canonical run image

And The --stack argument to pack create-builder is no longer accepted

Notes

• Ignore the [[stacks]] list in config.toml.

[ameyer-pivotal]

@sclevine Is it assumed that the "canonical run image" is the one specified in config.toml ("image" field, alongside locally-defined mirrors)? Wonder if the order: canonical, locally-defined mirrors, mirrors from builder would make more sense or be more similar to current functionality.

cc: @ekcasey

[sclevine]

The canonical run image is the run image reference that's used as the ID to join the entries in config.toml and the builder metadata. run-images[].image in config.toml contains reference.

The locally-defined mirrors should override this value if they use the same registry. Not sure about the builder-defined values, but I think those should go first for consistency with the other mirrors.

[ameyer-pivotal]

@sclevine, @ekcasey It turns out that because #38 was already played, the search order logic is already in place. However, it's actually different from what this story says (it has the canonical image in the middle, rather than being last):

i := append(overrideRunImages, append([]string{builderMetadata.RunImage.Image}, builderMetadata.RunImage.Mirrors...)...)
b.RunImage, err = config.ImageByRegistry(reg, i)

Do we feel strongly enough for me to make the change? Leave as is? Or change as part of another story (or perhaps reject #38)?

[ameyer-pivotal]

@sclevine Also, I think we're going to need to hold off on removing the stack-related CLI commands and do that as part of #48, as rebase still needs access to the stack info in the local config. It would make more sense to do it there.

[sclevine]

re: search order logic, I think we should keep the canonical image last, as part of this story.

Image this scenario:

1. As a builder author, I reference a standard run image that other builders use
2. However, I want my builder users to pick up a run image that I update at a different frequency than the canonical image
3. Both the canonical image and my run image live on the same registry.

re: CLI stack commands, agree that we should put off until #48

[ameyer-pivotal]

@sclevine Spoke with the team this morning and we decided to leave the order as is for now. We can chat more when you're back though.

[mgibson1121]

This did not pass acceptance for the following reasons:

1. There doesn't seem to be validation on the three mandatory fields, I can successfully build the builder w/o them. We tested this by commenting out these fields individually and trying to build the builder.

2. Attempting to build an app using an image generated by the builder causes an unintelligible error (panic)

Matthews-MacBook-Pro-2:testing mgibson1121$ pack build --builder my-builder my-image --no-pull
Defaulting app directory to current working directory /Users/mgibson1121/Desktop/testing (use --path to override)
Using user-provided builder image my-builder
panic: runtime error: index out of range

goroutine 1 [running]:
github.com/buildpack/pack/config.ImageByRegistry(0x155484a, 0xf, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x1b)
	/Users/travis/gopath/src/github.com/buildpack/pack/config/config.go:210 +0x131
github.com/buildpack/pack.(*BuildFactory).BuildConfigFromFlags(0xc0002d0600, 0xc0002ef9d0, 0x0, 0x0, 0xc000236f00)
	/Users/travis/gopath/src/github.com/buildpack/pack/build.go:183 +0xf15
main.buildCommand.func1(0xc0002e1180, 0xc0002d05c0, 0x1, 0x4, 0xc0001e5c48, 0xc0001e5c18)
	/Users/travis/gopath/src/github.com/buildpack/pack/cmd/pack/main.go:73 +0x9a
main.logError.func1(0xc0002e1180, 0xc0002d05c0, 0x1, 0x4, 0x0, 0x0)
	/Users/travis/gopath/src/github.com/buildpack/pack/cmd/pack/main.go:412 +0x5a
github.com/spf13/cobra.(*Command).execute(0xc0002e1180, 0xc0002d0580, 0x4, 0x4, 0xc0002e1180, 0xc0002d0580)
	/Users/travis/gopath/pkg/mod/github.com/spf13/cobra@v0.0.3/command.go:762 +0x473
github.com/spf13/cobra.(*Command).ExecuteC(0xc0002e0f00, 0x7, 0x0, 0xc000304c80)
	/Users/travis/gopath/pkg/mod/github.com/spf13/cobra@v0.0.3/command.go:852 +0x2fd
github.com/spf13/cobra.(*Command).Execute(0xc0002e0f00, 0xc0001e5f28, 0x1)
	/Users/travis/gopath/pkg/mod/github.com/spf13/cobra@v0.0.3/command.go:800 +0x2b
main.main()
	/Users/travis/gopath/src/github.com/buildpack/pack/cmd/pack/main.go:56 +0x2da
Matthews-MacBook-Pro-2:testing mgibson1121$

We believe all of the builder.toml arguments were provided.

[mgibson1121]

@ameyer-pivotal You mentioned during IPM that you wanted to see some of the files on my local machine to help debug the above error. Could you specify what you need? I was planning on providing the builder.toml, buildpack toml, and config.toml I used.

[ameyer-pivotal]

@mgibson1121 That should be good, thanks!

[mgibson1121]

@ameyer-pivotal Github won't let me upload toml files, so made copies and changed them to txt. I'm pretty sure these are the ones Stephen and I used, but let me know if something seems off.
builder copy.txt
config copy.txt
buildpack.txt

[mgibson1121]

@sclevine I accepted validation on the mandatory fields. The behavior is if one required field is left out, an error specifying that field is required is thrown. If more than one required field is not included, the error only displays that the stack id is required. This behavior is acceptable to me in the absence of more defined AC.

Additionally, weren't able to reproduce the panic on my machine. Accepting the story.

[sclevine]

@djoyahoy The panic appears to be due to a missing length check here: https://github.com/buildpack/pack/blob/master/config/config.go#L235

[ameyer-pivotal]

@mgibson1121 @sclevine Should we reject this then?

[mgibson1121]

@ameyer-pivotal My personal opinion is that we should only reject it if this bug was introduced by work on this story. If not, we should pass it and file a bug ticket. Up to @sclevine, though.

[ameyer-pivotal]

@mgibson1121 I agree that a separate story seems appropriate here. Can you create a new bug story and reference Stephen's comment: #47 (comment)

[mgibson1121]

Bug ticket split off from this ticket in #52. Will keep this story in the 0.1.0 release column.