Fix remoteManifests #2258
Merged
Fix remoteManifests #2258
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
tanner-bruce
requested review from
balopat,
dgageot,
nkubala,
priyawadhwa and
tejal29
as code owners
tanner-bruce
force-pushed
the
fix-remote-manifests
branch
from
cdfd10a to
6f3d880
Compare
|
Would love to get this merged in as soon as possible. We want to add skaffold to our development workflow, but due to our current setup this is a requirement before it would make sense to move over. |
Codecov Report
|
nkubala
previously requested changes
Jun 19, 2019
There was a problem hiding this comment.
@tanner-bruce thanks for the PR! can you add some tests for this? ideally for both the deployer change and the image replacer change.
|
@tanner-bruce any chance you could update this PR? |
tanner-bruce
force-pushed
the
fix-remote-manifests
branch
2 times, most recently
from
9e6593d to
4b533b4
Compare
This commit address issue GoogleContainerTools#2160. Remote manifests in the config are properly pulled again. This also addresses another unmentioned issue: when using a remote manifest with `skaffold dev`, the remote manifest will have its image updated to include the new tag. When shutting down, the remote manifest will be left the same, including the updated tag. This causes further runs of `skaffold dev` to no longer match the image, and not update it properly. This is addressed by saving all of the images in the manifests found the first time apply is ran, and then on cleanup replacing the images with those initially found.
Adds a test ensuring the proper sequence of commands for reading and cleaning up remote manifests is executed
tejal29
force-pushed
the
fix-remote-manifests
branch
from
4b533b4 to
983a7db
Compare
|
@tanner-bruce i added another test case which you missed. |
tejal29
approved these changes
Aug 9, 2019
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 commit address issue #2160. Remote manifests in the config are
properly pulled again.
This also addresses another unmentioned issue: when using a remote
manifest with
skaffold dev, the remote manifest will have its imageupdated to include the new tag. When shutting down, the remote manifest
will be left the same, including the updated tag. This causes further
runs of
skaffold devto no longer match the image, and not update itproperly.
This is addressed by saving all of the images in the manifests found the
first time apply is ran, and then on cleanup replacing the images with
those initially found.