Skip to content

Preserve sync subtree for '***'.#1813

Merged
tejal29 merged 6 commits intoGoogleContainerTools:masterfrom
michaelfig:sync-subtree
Mar 19, 2019
Merged

Preserve sync subtree for '***'.#1813
tejal29 merged 6 commits intoGoogleContainerTools:masterfrom
michaelfig:sync-subtree

Conversation

@michaelfig
Copy link
Copy Markdown
Contributor

Fixes #1807

This PR implements subtree syncing for '***' globs in the sync specification.

Given:

    sync:
      'foo/***/*.js': /usr/src/app/

and changing foo/baz/bot/test.js, test.js will be synchronized to /usr/src/app/baz/bot/test.js.

Given:

    sync:
      '***': subdir

and changing my/src.txt, src.txt will be synchronized to ${WORKDIR}/subdir/my/src.txt.

All the existing sync specs in 0.25.0 mean the same as they did before this PR (i.e. they collapse the subPath, so all files in the subtree are synced to the same destination directory, not its children). To allow for backward compatibility with 0.23.0's subtree syncing, you can use:

    sync:
      '**': /usr/src/app/ # v0.23.0
      '***': /usr/src/app/ # this PR

The line marked this PR will take precedence over the v0.23.0 line (triple-stars are matched first before other patterns), except when running a version of Skaffold before this PR. In that case, the v0.23.0 line will be used, and only v0.23.0 will produce the same behaviour as this PR.

Hope this helps,
Michael.

if isTripleStar {
// Map the paths as a tree from the prefix.
subtreePrefix := strings.Split(p, "***")[0]
subPath = strings.TrimPrefix(relPath, subtreePrefix)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Need:

filepath.FromSlash(subtreePrefix)

Otherwise the prefix-pruning fails on Windows.

@codecov-io
Copy link
Copy Markdown

codecov-io commented Mar 16, 2019

Codecov Report

Merging #1813 into master will increase coverage by 0.2%.
The diff coverage is 85.41%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #1813     +/-   ##
=========================================
+ Coverage   45.54%   45.75%   +0.2%     
=========================================
  Files         143      143             
  Lines        6683     6723     +40     
=========================================
+ Hits         3044     3076     +32     
- Misses       3333     3338      +5     
- Partials      306      309      +3
Impacted Files Coverage Δ
pkg/skaffold/sync/sync.go 74.38% <85.41%> (+3.79%) ⬆️
integration/util.go 0% <0%> (ø) ⬆️
pkg/skaffold/initializer/init.go 7.64% <0%> (+1.19%) ⬆️

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 ba77865...02da1e0. Read the comment docs.

@tejal29 tejal29 added the kokoro:run runs the kokoro jobs on a PR label Mar 18, 2019
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Mar 18, 2019
@tejal29 tejal29 added the kokoro:run runs the kokoro jobs on a PR label Mar 18, 2019
@tejal29
Copy link
Copy Markdown
Contributor

tejal29 commented Mar 18, 2019

Thank you for the bug fix! The kokoro job failed due to intergration test flakiness #1538.
This should be fixed. I have the kokoro-run label. Hopefully a new build will be kicked off soon.

@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Mar 19, 2019
@michaelfig
Copy link
Copy Markdown
Contributor Author

@tejal29 I implemented your proposed changes. I found and fixed some test failures with absolute paths on Windows. Would you mind please relabeling kokoro-run?

@tejal29 tejal29 added the kokoro:run runs the kokoro jobs on a PR label Mar 19, 2019
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Mar 19, 2019
doubleStarPattern[p] = dst
}
}
return
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks, i never knew you can return like this in go.

@tejal29 tejal29 merged commit 12bf986 into GoogleContainerTools:master Mar 19, 2019
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.

5 participants