Skip to content
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.

Sign up for GitHub

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

Pass --volume options to creator phase #675

Merged
merged 4 commits into from
Jun 8, 2020
Merged

Pass --volume options to creator phase #675

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3ebcac1
Pass --volume options to creator phase
simonjjones Jun 5, 2020
7176afe
Skip volume mounting test for pack version with volume mounting bug
simonjjones Jun 5, 2020
1f10e08
Ignore potential cleanup errors now test is skippable
simonjjones Jun 5, 2020
a52dbcc
Remove duplicated test for generic bind mounts
simonjjones Jun 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 13 additions & 6 deletions acceptance/acceptance_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -933,9 +933,16 @@ func testAcceptance(
var buildpackTgz, tempVolume string

it.Before(func() {
packVer, err := packVersion(packPath)
h.AssertNil(t, err)
packSemver := semver.MustParse(strings.TrimPrefix(strings.Split(packVer, " ")[0], "v"))
h.SkipIf(t,
packSemver.Equal(semver.MustParse("0.11.0")),
"pack 0.11.0 shipped with a volume mounting bug",
)

buildpackTgz = h.CreateTGZ(t, filepath.Join(bpDir, "volume-buildpack"), "./", 0755)

var err error
tempVolume, err = ioutil.TempDir("", "my-volume-mount-source")
h.AssertNil(t, err)
h.AssertNil(t, os.Chmod(tempVolume, 0755)) // Override umask
Expand All @@ -950,10 +957,10 @@ func testAcceptance(
})

it.After(func() {
h.AssertNil(t, os.Remove(buildpackTgz))
h.AssertNil(t, h.DockerRmi(dockerCli, repoName))
_ = os.Remove(buildpackTgz)
_ = h.DockerRmi(dockerCli, repoName)

h.AssertNil(t, os.RemoveAll(tempVolume))
_ = os.RemoveAll(tempVolume)
Comment on lines +960 to +963
Copy link
Contributor

@yaelharel yaelharel Jun 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Why did you remove the assertions?

Copy link
Member Author

Choose a reason for hiding this comment

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

The test is now skippable, so cleanup can reasonably expect to fail when it's skipped

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right! Thanks for the clarification!

})

it("mounts the provided volume in the detect and build phases", func() {
Expand All @@ -965,9 +972,9 @@ func testAcceptance(
))

if packSemver.GreaterThan(semver.MustParse("0.9.0")) || packSemver.Equal(semver.MustParse("0.0.0")) {
h.AssertContains(t, output, "Detect: Reading file '/platform/my-volume-mount-target/some-file':")
h.AssertContains(t, output, "Detect: Reading file '/platform/my-volume-mount-target/some-file': some-string")
}
h.AssertContains(t, output, "Build: Reading file '/platform/my-volume-mount-target/some-file':")
h.AssertContains(t, output, "Build: Reading file '/platform/my-volume-mount-target/some-file': some-string")
})
})

Expand Down
13 changes: 12 additions & 1 deletion internal/build/build.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,18 @@ func (l *Lifecycle) Execute(ctx context.Context, opts LifecycleOptions) error {
return l.Export(ctx, opts.Image.Name(), opts.RunImage, opts.Publish, launchCache.Name(), buildCache.Name(), opts.Network, phaseFactory)
}

return l.Create(ctx, opts.Publish, opts.ClearCache, opts.RunImage, launchCache.Name(), buildCache.Name(), opts.Image.Name(), opts.Network, phaseFactory)
return l.Create(
ctx,
opts.Publish,
opts.ClearCache,
opts.RunImage,
launchCache.Name(),
buildCache.Name(),
opts.Image.Name(),
opts.Network,
opts.Volumes,
phaseFactory,
)
}

func (l *Lifecycle) Setup(opts LifecycleOptions) {
Expand Down
9 changes: 7 additions & 2 deletions internal/build/phases.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,15 +36,20 @@ type PhaseFactory interface {
New(provider *PhaseConfigProvider) RunnerCleaner
}

func (l *Lifecycle) Create(ctx context.Context, publish, clearCache bool, runImage, launchCacheName, cacheName, repoName, networkMode string, phaseFactory PhaseFactory) error {
func (l *Lifecycle) Create(
ctx context.Context,
publish, clearCache bool,
runImage, launchCacheName, cacheName, repoName, networkMode string,
volumes []string,
phaseFactory PhaseFactory) error {
var configProvider *PhaseConfigProvider

args := []string{
"-run-image", runImage,
repoName,
}

binds := []string{fmt.Sprintf("%s:%s", cacheName, cacheDir)}
binds := append(volumes, fmt.Sprintf("%s:%s", cacheName, cacheDir))

if clearCache {
args = append([]string{"-skip-restore"}, args...)
Expand Down
167 changes: 156 additions & 11 deletions internal/build/phases_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,18 @@ func testPhases(t *testing.T, when spec.G, it spec.S) {
fakePhase := &fakes.FakePhase{}
fakePhaseFactory := fakes.NewFakePhaseFactory(fakes.WhichReturnsForNew(fakePhase))

err := lifecycle.Create(context.Background(), false, false, "test", "test", "test", "test", "test", fakePhaseFactory)
err := lifecycle.Create(
context.Background(),
false,
false,
"test",
"test",
"test",
"test",
"test",
[]string{},
fakePhaseFactory,
)
h.AssertNil(t, err)

h.AssertEq(t, fakePhase.CleanupCallCount, 1)
Expand All @@ -70,7 +81,18 @@ func testPhases(t *testing.T, when spec.G, it spec.S) {
expectedRepoName := "some-repo-name"
expectedRunImage := "some-run-image"

err := verboseLifecycle.Create(context.Background(), false, false, expectedRunImage, "test", "test", expectedRepoName, "test", fakePhaseFactory)
err := verboseLifecycle.Create(
context.Background(),
false,
false,
expectedRunImage,
"test",
"test",
expectedRepoName,
"test",
[]string{},
fakePhaseFactory,
)
h.AssertNil(t, err)

configProvider := fakePhaseFactory.NewCalledWithProvider
Expand All @@ -88,19 +110,65 @@ func testPhases(t *testing.T, when spec.G, it spec.S) {
fakePhaseFactory := fakes.NewFakePhaseFactory()
expectedNetworkMode := "some-network-mode"

err := lifecycle.Create(context.Background(), false, false, "test", "test", "test", "test", expectedNetworkMode, fakePhaseFactory)
err := lifecycle.Create(
context.Background(),
false,
false,
"test",
"test",
"test",
"test",
expectedNetworkMode,
[]string{},
fakePhaseFactory,
)
h.AssertNil(t, err)

configProvider := fakePhaseFactory.NewCalledWithProvider
h.AssertEq(t, configProvider.HostConfig().NetworkMode, container.NetworkMode(expectedNetworkMode))
})

it("configures the phase with both cache binds and custom volume mounts", func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you added this check, maybe we can remove the tests on lines 212 and 332

Copy link
Member Author

Choose a reason for hiding this comment

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

They still test slightly different circumstances - the difference between publish and not, as well as ensuring the cache binds will be configured regardless of whether a custom volume is configured.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right but I'm not sure if I would keep all 3 tests. I think it's a little confusing that we're testing part of it 3 times.
Maybe we change this test to test only the configuration with custom volume mounts, and to keep the other tests to test only the cache and launch-cache.
What do you think about that?

Copy link
Member Author

Choose a reason for hiding this comment

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

I opted to remove this test, and add custom volume mounts to the existing bind tests which cover both the publish=true and publish=false contexts.

lifecycle := newTestLifecycle(t, false)
fakePhaseFactory := fakes.NewFakePhaseFactory()
expectedBind := "some-mount-source:/some-mount-target"
expectedBinds := []string{expectedBind, "some-cache:/cache", "some-launch-cache:/launch-cache"}

err := lifecycle.Create(
context.Background(),
false,
false,
"test",
"some-launch-cache",
"some-cache",
"test",
"test",
[]string{expectedBind},
fakePhaseFactory,
)
h.AssertNil(t, err)

configProvider := fakePhaseFactory.NewCalledWithProvider
h.AssertSliceContains(t, configProvider.HostConfig().Binds, expectedBinds...)
})

when("clear cache", func() {
it("configures the phase with the expected arguments", func() {
verboseLifecycle := newTestLifecycle(t, true)
fakePhaseFactory := fakes.NewFakePhaseFactory()

err := verboseLifecycle.Create(context.Background(), false, true, "test", "test", "test", "test", "test", fakePhaseFactory)
err := verboseLifecycle.Create(
context.Background(),
false,
true,
"test",
"test",
"test",
"test",
"test",
[]string{},
fakePhaseFactory,
)
h.AssertNil(t, err)

configProvider := fakePhaseFactory.NewCalledWithProvider
Expand All @@ -117,7 +185,18 @@ func testPhases(t *testing.T, when spec.G, it spec.S) {
verboseLifecycle := newTestLifecycle(t, true)
fakePhaseFactory := fakes.NewFakePhaseFactory()

err := verboseLifecycle.Create(context.Background(), false, false, "test", "test", "test", "test", "test", fakePhaseFactory)
err := verboseLifecycle.Create(
context.Background(),
false,
false,
"test",
"test",
"test",
"test",
"test",
[]string{},
fakePhaseFactory,
)
h.AssertNil(t, err)

configProvider := fakePhaseFactory.NewCalledWithProvider
Expand All @@ -135,7 +214,18 @@ func testPhases(t *testing.T, when spec.G, it spec.S) {
fakePhaseFactory := fakes.NewFakePhaseFactory()
expectedBinds := []string{"some-cache:/cache"}

err := lifecycle.Create(context.Background(), true, false, "test", "test", "some-cache", "test", "test", fakePhaseFactory)
err := lifecycle.Create(
context.Background(),
true,
false,
"test",
"test",
"some-cache",
"test",
"test",
[]string{},
fakePhaseFactory,
)
h.AssertNil(t, err)

configProvider := fakePhaseFactory.NewCalledWithProvider
Expand All @@ -146,7 +236,18 @@ func testPhases(t *testing.T, when spec.G, it spec.S) {
lifecycle := newTestLifecycle(t, false)
fakePhaseFactory := fakes.NewFakePhaseFactory()

err := lifecycle.Create(context.Background(), true, false, "test", "test", "test", "test", "test", fakePhaseFactory)
err := lifecycle.Create(
context.Background(),
true,
false,
"test",
"test",
"test",
"test",
"test",
[]string{},
fakePhaseFactory,
)
h.AssertNil(t, err)

configProvider := fakePhaseFactory.NewCalledWithProvider
Expand All @@ -158,7 +259,18 @@ func testPhases(t *testing.T, when spec.G, it spec.S) {
fakePhaseFactory := fakes.NewFakePhaseFactory()
expectedRepos := "some-repo-name"

err := lifecycle.Create(context.Background(), true, false, "test", "test", "test", expectedRepos, "test", fakePhaseFactory)
err := lifecycle.Create(
context.Background(),
true,
false,
"test",
"test",
"test",
expectedRepos,
"test",
[]string{},
fakePhaseFactory,
)
h.AssertNil(t, err)

configProvider := fakePhaseFactory.NewCalledWithProvider
Expand All @@ -171,7 +283,18 @@ func testPhases(t *testing.T, when spec.G, it spec.S) {
verboseLifecycle := newTestLifecycle(t, true)
fakePhaseFactory := fakes.NewFakePhaseFactory()

err := verboseLifecycle.Create(context.Background(), false, false, "test", "test", "test", "test", "test", fakePhaseFactory)
err := verboseLifecycle.Create(
context.Background(),
false,
false,
"test",
"test",
"test",
"test",
"test",
[]string{},
fakePhaseFactory,
)
h.AssertNil(t, err)

configProvider := fakePhaseFactory.NewCalledWithProvider
Expand All @@ -187,7 +310,18 @@ func testPhases(t *testing.T, when spec.G, it spec.S) {
lifecycle := newTestLifecycle(t, false)
fakePhaseFactory := fakes.NewFakePhaseFactory()

err := lifecycle.Create(context.Background(), false, false, "test", "some-launch-cache", "some-cache", "test", "test", fakePhaseFactory)
err := lifecycle.Create(
context.Background(),
false,
false,
"test",
"some-launch-cache",
"some-cache",
"test",
"test",
[]string{},
fakePhaseFactory,
)
h.AssertNil(t, err)

configProvider := fakePhaseFactory.NewCalledWithProvider
Expand All @@ -200,7 +334,18 @@ func testPhases(t *testing.T, when spec.G, it spec.S) {
fakePhaseFactory := fakes.NewFakePhaseFactory()
expectedBinds := []string{"some-cache:/cache", "some-launch-cache:/launch-cache"}

err := lifecycle.Create(context.Background(), false, false, "test", "some-launch-cache", "some-cache", "test", "test", fakePhaseFactory)
err := lifecycle.Create(
context.Background(),
false,
false,
"test",
"some-launch-cache",
"some-cache",
"test",
"test",
[]string{},
fakePhaseFactory,
)
h.AssertNil(t, err)

configProvider := fakePhaseFactory.NewCalledWithProvider
Expand Down