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.

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

Fix bug in sync #1709

Merged
merged 8 commits into from
Mar 1, 2019
Merged

Conversation

priyawadhwa
Copy link
Contributor

@priyawadhwa priyawadhwa commented Feb 28, 2019

This PR fixes a bug in sync, in which desinations for synced files were
being calculated incorrectly. For the hot-reload example, app.py was
being synced to /src/app.py instead of /app.py, so it appeared as if
sync wasn't actually working.

To fix this issue, I fixed a bug in the intersect function to make sure
destination calculation follows the Docker COPY command rules.

For example, we had a unit test as follows:

syncMap: **/**/*.js": "."
file: dir1/dir2/node.js
dst: /dir1/dir2/node.js

I think the destination should actually be /node.js based on Docker COPY
command rules.

should fix #1668

This PR fixes a bug in sync, in which desinations for synced files were
being calculated incorrectly. For the hot-reload example, app.py was
being synced to /src/app.py instead of /app.py, so it appeared as if
sync wasn't actually working.

To fix this issue, I fixed a bug in the intersect function to make sure
destination calculation follows the Docker COPY command rules.

For example, we had a unit test as follows:

syncMap: **/**/*.js": "."
file: dir1/dir2/node.js
dst: /dir1/dir2/node.js

when the destination should actually be /node.js based on Docker COPY
command rules.
@codecov-io
Copy link

Codecov Report

Merging #1709 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1709   +/-   ##
=======================================
  Coverage   47.05%   47.05%           
=======================================
  Files         126      126           
  Lines        6142     6142           
=======================================
  Hits         2890     2890           
  Misses       2961     2961           
  Partials      291      291
Impacted Files Coverage Δ
pkg/skaffold/sync/sync.go 91.3% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd485f6...b7ef0c7. Read the comment docs.

@codecov-io
Copy link

codecov-io commented Feb 28, 2019

Codecov Report

Merging #1709 into master will decrease coverage by 0.14%.
The diff coverage is 56.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1709      +/-   ##
==========================================
- Coverage   47.05%   46.91%   -0.15%     
==========================================
  Files         126      126              
  Lines        6142     6167      +25     
==========================================
+ Hits         2890     2893       +3     
- Misses       2961     2976      +15     
- Partials      291      298       +7
Impacted Files Coverage Δ
pkg/skaffold/docker/image.go 58.38% <0%> (ø) ⬆️
pkg/skaffold/docker/remote.go 24.24% <0%> (ø) ⬆️
pkg/skaffold/sync/sync.go 75.28% <60%> (-16.03%) ⬇️
hack/schemas/main.go 87% <0%> (-3.12%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd485f6...dc3131c. Read the comment docs.

Copy link
Contributor

@dgageot dgageot left a comment

Choose a reason for hiding this comment

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

I'm not sure I agree. Let me think about it before it's merged

@dgageot
Copy link
Contributor

dgageot commented Mar 1, 2019

There seems to be an issue with name.ParseReference(target, name.WeakValidation). It fails to parse gcr.io/k8s-skaffold/test-file-sync:67738fe@sha256:13e30612be9aa5dae72fb06619ff0dabf80ca600e8f949da683abe73d2f0113b even if it's a correct image reference.

cc @jonjohnsonjr

since go-containerregistry can't currently handle both
@priyawadhwa
Copy link
Contributor Author

@dgageot @balopat I just pushed a workaround to remove the tag and use just the digest for getting the workingDir from the image until go-containerregistry can parse both.

@nkubala
Copy link
Contributor

nkubala commented Mar 1, 2019

@priyawadhwa I think that's fine, can you check out the failing test in Kokoro though? doesn't look like a flake

@jonjohnsonjr
Copy link
Contributor

google/go-containerregistry#351

I kind of have a philosophical disagreement with that format of reference. I think it's misleading.

I'm happy to take a PR to fix it, I just don't have time to figure out the regex voodoo myself.

@priyawadhwa priyawadhwa merged commit 11803e9 into GoogleContainerTools:master Mar 1, 2019
@priyawadhwa priyawadhwa deleted the syncbug branch March 1, 2019 19:06
priyawadhwa pushed a commit to priyawadhwa/skaffold-1 that referenced this pull request Mar 4, 2019
In GoogleContainerTools#1709 I introduced a regression to sync by removing the "-C /" from
the tar extraction, which forced all files to be extracted at root. We
actually still want this, since now that we append the workdir for relative paths  all paths in the
tarball are absolute paths and so should be extracted at root.

This should fix GoogleContainerTools#1721.
priyawadhwa pushed a commit to priyawadhwa/skaffold-1 that referenced this pull request Mar 4, 2019
In GoogleContainerTools#1709 I introduced a regression to sync by removing the "-C /" from
the tar extraction, which forced all files to be extracted at root. We
actually still want this, since now that we append the workdir for relative paths  all paths in the
tarball are absolute paths and so should be extracted at root.

This should fix GoogleContainerTools#1721.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Skaffold v0.23.0 + Windows 10 regression: didn't sync any files
7 participants