Pass --volume options to creator phase #675
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Resolves #674 Signed-off-by: Simon Jones <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #675 +/- ##
==========================================
- Coverage 72.65% 72.49% -0.16%
==========================================
Files 67 67
Lines 4866 4877 +11
==========================================
Hits 3535 3535
- Misses 1010 1021 +11
Partials 321 321
|
Signed-off-by: Simon Jones <[email protected]>
Signed-off-by: Simon Jones <[email protected]>
yaelharel
reviewed
Jun 5, 2020
Comment on lines
+960
to
+963
| _ = os.Remove(buildpackTgz) | ||
| _ = h.DockerRmi(dockerCli, repoName) | ||
|
|
||
| h.AssertNil(t, os.RemoveAll(tempVolume)) | ||
| _ = os.RemoveAll(tempVolume) |
There was a problem hiding this comment.
Why did you remove the assertions?
There was a problem hiding this comment.
The test is now skippable, so cleanup can reasonably expect to fail when it's skipped
There was a problem hiding this comment.
You're right! Thanks for the clarification!
internal/build/phases_test.go
Outdated
| 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() { |
There was a problem hiding this comment.
Since you added this check, maybe we can remove the tests on lines 212 and 332
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Signed-off-by: Simon Jones <[email protected]>
yaelharel
approved these changes
Jun 5, 2020
✔️ Accepted |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This resolves a bug where we weren't configuring the
creatorphase with any binds configured with the--volumeoptionResolves #674
Signed-off-by: Simon Jones [email protected]