-
Notifications
You must be signed in to change notification settings - Fork 107
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dockerfiles phase 1 (generate changes) #869
Conversation
Signed-off-by: Natalie Arellano <[email protected]>
Apologies in advance for the huge diff - this is built off of #867. If/when that is merged, the diff should be smaller ...but still huge. Edit: re-pointing the branch for now. |
3dd721b
to
5c2bf0e
Compare
Logger: &log.Logger{Handler: logHandler}, | ||
} | ||
when("#Build", func() { | ||
when("env", func() { |
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.
This file was getting a little unwieldy, hence the huge diff. I'll add some comments to try to make it more understandable.
TODO: need to gate this on experimental features enabled |
Generate changes to support Dockerfiles feature Signed-off-by: Natalie Arellano <[email protected]>
5c2bf0e
to
6757d25
Compare
Mainly: the creation of a group-extensions field in group.toml Signed-off-by: Natalie Arellano <[email protected]>
Signed-off-by: Natalie Arellano <[email protected]>
Signed-off-by: Natalie Arellano <[email protected]>
Signed-off-by: Natalie Arellano <[email protected]>
when("for extension", func() { | ||
it.Before(func() { | ||
h.SkipIf(t, kind == buildpack.KindBuildpack, "") | ||
}) | ||
|
||
when("dockerfiles", func() { | ||
it("includes run.Dockerfile", func() { | ||
h.Mkfile(t, | ||
"", | ||
filepath.Join(appDir, "run.Dockerfile-A-v1"), | ||
) | ||
|
||
br, err := descriptor.Build(buildpack.Plan{}, config, mockEnv) | ||
h.AssertNil(t, err) | ||
|
||
h.AssertEq(t, br.Dockerfiles[0].ExtensionID, "A") | ||
h.AssertEq(t, br.Dockerfiles[0].Kind, buildpack.DockerfileKindRun) | ||
h.AssertEq(t, br.Dockerfiles[0].Path, filepath.Join(layersDir, "A", "run.Dockerfile")) | ||
}) | ||
}) | ||
|
||
when("/bin/build is missing", func() { | ||
it.Before(func() { | ||
descriptor.Extension.ID = "B" | ||
descriptor.Dir = filepath.Join(storeDir, "B", "v1") | ||
}) | ||
|
||
it("treats the extension root as a pre-populated output directory", func() { | ||
bpPlan := buildpack.Plan{ | ||
Entries: []buildpack.Require{ | ||
{Name: "some-dep"}, | ||
{Name: "some-other-dep"}, | ||
{Name: "some-unmet-dep"}, | ||
}, | ||
} | ||
|
||
br, err := descriptor.Build(bpPlan, config, mockEnv) | ||
h.AssertNil(t, err) | ||
|
||
t.Log("processes build.toml") | ||
h.AssertEq(t, br.MetRequires, []string{"some-dep", "some-other-dep"}) | ||
t.Log("processes run.Dockerfile") | ||
h.AssertEq(t, br.Dockerfiles[0].ExtensionID, "B") | ||
h.AssertEq(t, br.Dockerfiles[0].Kind, buildpack.DockerfileKindRun) | ||
h.AssertEq(t, br.Dockerfiles[0].Path, filepath.Join(descriptor.Dir, "run.Dockerfile")) | ||
}) |
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.
This is the most relevant test
if kind == buildpack.KindExtension { | ||
t.Log("sets CNB_OUTPUT_DIR") | ||
actual = h.Rdfile(t, filepath.Join(appDir, "build-env-cnb-output-dir-A-v1.clear")) | ||
h.AssertEq(t, actual, filepath.Join(layersDir, "A")) | ||
t.Log("does not set CNB_LAYERS_DIR") | ||
actual = h.Rdfile(t, filepath.Join(appDir, "build-env-cnb-layers-dir-A-v1.clear")) | ||
h.AssertEq(t, isUnset(actual), true) | ||
} else { | ||
t.Log("sets CNB_LAYERS_DIR") | ||
actual = h.Rdfile(t, filepath.Join(appDir, "build-env-cnb-layers-dir-A-v1.clear")) | ||
h.AssertEq(t, actual, filepath.Join(layersDir, "A")) | ||
t.Log("does not set CNB_OUTPUT_DIR") | ||
actual = h.Rdfile(t, filepath.Join(appDir, "build-env-cnb-output-dir-A-v1.clear")) | ||
h.AssertEq(t, isUnset(actual), true) | ||
} |
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.
Also relevant
Signed-off-by: Natalie Arellano <[email protected]>
Signed-off-by: Natalie Arellano <[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.
Looks good!
Edit: resolved the TODOs I think.
Together with #860 this should close out #849.