Add configuration option for sync inference [3/3] #2088

corneliusweig · 2019-05-07T23:28:34Z

The sync feature is getting refurbished, see design proposal #1844 .

This PR implements step 3 of the implementation plan for the sync improvements. Notable changes include:

add a new config option sync.infer []string to enable sync for some files
add unit tests for creation of sync items
add integration test for inferred sync mode

based-on #2084
Supersedes #1812
Fixes #1166, #1581
Related #1180, #1807
CC design shepherd @tejal29

codecov-io · 2019-05-08T10:46:28Z

Codecov Report

Merging #2088 into master will decrease coverage by <.01%.
The diff coverage is 80.95%.

Impacted Files	Coverage Δ
pkg/skaffold/schema/latest/config.go	`100% <ø> (ø)`	⬆️
pkg/skaffold/docker/parse.go	`88.33% <100%> (ø)`	⬆️
pkg/skaffold/runner/dev.go	`65.65% <50%> (-0.67%)`	⬇️
pkg/skaffold/sync/sync.go	`81.89% <82.05%> (-1.44%)`	⬇️
pkg/skaffold/event/event.go	`88.43% <0%> (-1.37%)`	⬇️
pkg/skaffold/jib/jib.go	`77.41% <0%> (-1.08%)`	⬇️

container-tools-bot · 2019-05-10T23:12:45Z

Please visit http://35.236.44.79:1313 to view changes to the docs.

corneliusweig · 2019-06-25T21:09:45Z

@tejal29 @balopat Just a polite reminder that sync inference is still not complete due to the missing skaffold.yaml adaptions from this PR.

dgageot · 2019-07-02T05:27:26Z

@corneliusweig What's the status on this PR? Is it good to be reviewed or are you still working on it? I'm happy to review

corneliusweig · 2019-07-02T05:34:58Z

@dgageot it is ready. Thanks for taking a look!

dgageot

LGTM with just a few nits. It would be better if someone else could review too

dgageot · 2019-07-02T09:00:45Z

docs/content/en/docs/how-tos/filesync/_index.md

@@ -15,6 +15,37 @@ The file copying is enabled by adding a `sync` section with _sync rules_ to the
 Under the hood, Skaffold creates a tar file with changed files that match the sync rules.
 This tar file is sent to and extracted on the corresponding containers. 

+### Inferred sync mode
+For docker artifacts, Skaffold knows to infer the desired destination from the artifact's `Dockerfile`.


knows -> knows how?

corneliusweig · 2019-07-15T09:07:34Z

@tejal29 As you were the design shepherd: could you also review? Or find somebody to do that? Would be highly appreciated, as this PR has been ready for quite some time now.

nkubala

a few tiny nits but otherwise LGTM

pkg/skaffold/sync/sync.go

corneliusweig · 2019-07-22T21:03:45Z

@nkubala addressed your comments and rebased on master (there were conflicts)
And thanks for taking your time to review this huge PR!

nkubala · 2019-07-22T21:51:53Z

@corneliusweig looks good except I think there's an issue with the config file getting passed to TestDevSyncAPITrigger, it's looking for skaffold.yaml but I think you changed it to skaffold-infer.yaml

corneliusweig · 2019-07-22T22:16:02Z

@nkubala you are right. That config needed to be adapted. Now let's wait for travis.

corneliusweig · 2019-07-22T22:47:55Z

@nkubala CI is still failing. I now recall to also have refactored this test, because it had a loophole. I need to do the necessary adjustments tomorrow.

googlebot · 2019-07-23T09:40:50Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

- add new configuration sync.Infer - create sync Item for builds with inferred sync map - add test cases for inferred sync map Signed-off-by: Cornelius Weig <[email protected]>

Signed-off-by: Cornelius Weig <[email protected]>

Before, it was possible that the test would pass even though no sync had happened. Now the test can only pass, if the file is synced into the container. Signed-off-by: Cornelius Weig <[email protected]>

Signed-off-by: Cornelius Weig <[email protected]>

googlebot · 2019-07-23T15:26:54Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

corneliusweig · 2019-07-23T16:25:29Z

@nkubala Ok, CI is finally green again. There were some hickups.

nkubala · 2019-07-23T17:56:51Z

@corneliusweig awesome, thank you for pushing this through!

googlebot added the cla: yes label May 7, 2019

corneliusweig changed the title ~~WIP Add configuration option for sync inference~~ WIP Add configuration option for sync inference [3/3] May 7, 2019

corneliusweig changed the title ~~WIP Add configuration option for sync inference [3/3]~~ Add configuration option for sync inference [3/3] May 8, 2019

corneliusweig marked this pull request as ready for review May 8, 2019 10:32

corneliusweig requested review from balopat, dgageot, nkubala, priyawadhwa and tejal29 as code owners May 8, 2019 10:32

balopat added the needs-rebase label May 10, 2019

corneliusweig force-pushed the sync/step-3-1166 branch from 1ec92d2 to 99916c5 Compare May 10, 2019 21:42

corneliusweig mentioned this pull request May 10, 2019

Freeze v1beta10 config #2109

Merged

balopat added kokoro:run runs the kokoro jobs on a PR docs-modifications runs the docs preview service on the given PR and removed needs-rebase labels May 10, 2019

container-tools-bot removed the docs-modifications runs the docs preview service on the given PR label May 10, 2019

balopat added the !! config-change !! label May 10, 2019

kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label May 10, 2019

corneliusweig force-pushed the sync/step-3-1166 branch from 99916c5 to a2afe7a Compare May 23, 2019 10:47

corneliusweig force-pushed the sync/step-3-1166 branch 2 times, most recently from 605d047 to bd173e0 Compare June 11, 2019 22:23

corneliusweig force-pushed the sync/step-3-1166 branch from bd173e0 to fe719ad Compare June 15, 2019 21:26

corneliusweig force-pushed the sync/step-3-1166 branch 2 times, most recently from 80318d3 to 7f11844 Compare June 25, 2019 20:34

dgageot approved these changes Jul 2, 2019

View reviewed changes

nkubala approved these changes Jul 22, 2019

View reviewed changes

pkg/skaffold/sync/sync.go Show resolved Hide resolved

pkg/skaffold/sync/sync.go Outdated Show resolved Hide resolved

pkg/skaffold/sync/sync.go Outdated Show resolved Hide resolved

corneliusweig force-pushed the sync/step-3-1166 branch from 5924e40 to ebabc8c Compare July 22, 2019 21:02

corneliusweig force-pushed the sync/step-3-1166 branch from ebabc8c to 0875984 Compare July 22, 2019 22:14

corneliusweig force-pushed the sync/step-3-1166 branch from 4a4495c to ab8fa04 Compare July 23, 2019 09:05

googlebot added cla: no and removed cla: yes labels Jul 23, 2019

corneliusweig added 9 commits July 23, 2019 17:26

Implement inferred sync mode

52625e0

- add new configuration sync.Infer - create sync Item for builds with inferred sync map - add test cases for inferred sync map Signed-off-by: Cornelius Weig <[email protected]>

Use destination inference in the react-reload example

3dc88f6

Signed-off-by: Cornelius Weig <[email protected]>

Update docs for inferred sync mode

9253264

Signed-off-by: Cornelius Weig <[email protected]>

Add integration test for inferred filesync mode

53c3014

Signed-off-by: Cornelius Weig <[email protected]>

Fix loophole in integration test for filesync

860b15b

Before, it was possible that the test would pass even though no sync had happened. Now the test can only pass, if the file is synced into the container. Signed-off-by: Cornelius Weig <[email protected]>

Clarify that sync of file deletion cannot work in inferred sync mode

d5bc87d

Signed-off-by: Cornelius Weig <[email protected]>

Regenerate schemas after schema upgrade

e500506

Signed-off-by: Cornelius Weig <[email protected]>

More review comments

b210b27

Signed-off-by: Cornelius Weig <[email protected]>

Adapt TestDevSyncAPITrigger to improved test protocol

b32baac

Signed-off-by: Cornelius Weig <[email protected]>

corneliusweig force-pushed the sync/step-3-1166 branch from ab8fa04 to b32baac Compare July 23, 2019 15:26

googlebot added cla: yes and removed cla: no labels Jul 23, 2019

nkubala merged commit 7db7ce7 into GoogleContainerTools:master Jul 23, 2019

corneliusweig deleted the sync/step-3-1166 branch July 23, 2019 18:36

tejal29 mentioned this pull request Aug 22, 2019

Use Dockerfile file paths when using Sync #1581

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add configuration option for sync inference [3/3] #2088

Add configuration option for sync inference [3/3] #2088

corneliusweig commented May 7, 2019 •

edited

Loading

codecov-io commented May 8, 2019 •

edited by codecov bot

Loading

container-tools-bot commented May 10, 2019

corneliusweig commented Jun 25, 2019

dgageot commented Jul 2, 2019

corneliusweig commented Jul 2, 2019

dgageot left a comment

dgageot Jul 2, 2019

corneliusweig commented Jul 15, 2019

nkubala left a comment

corneliusweig commented Jul 22, 2019 •

edited

Loading

nkubala commented Jul 22, 2019

corneliusweig commented Jul 22, 2019

corneliusweig commented Jul 22, 2019

googlebot commented Jul 23, 2019

googlebot commented Jul 23, 2019

corneliusweig commented Jul 23, 2019

nkubala commented Jul 23, 2019

Add configuration option for sync inference [3/3] #2088

Add configuration option for sync inference [3/3] #2088

Conversation

corneliusweig commented May 7, 2019 • edited Loading

codecov-io commented May 8, 2019 • edited by codecov bot Loading

Codecov Report

container-tools-bot commented May 10, 2019

corneliusweig commented Jun 25, 2019

dgageot commented Jul 2, 2019

corneliusweig commented Jul 2, 2019

dgageot left a comment

Choose a reason for hiding this comment

dgageot Jul 2, 2019

Choose a reason for hiding this comment

corneliusweig commented Jul 15, 2019

nkubala left a comment

Choose a reason for hiding this comment

corneliusweig commented Jul 22, 2019 • edited Loading

nkubala commented Jul 22, 2019

corneliusweig commented Jul 22, 2019

corneliusweig commented Jul 22, 2019

googlebot commented Jul 23, 2019

googlebot commented Jul 23, 2019

corneliusweig commented Jul 23, 2019

nkubala commented Jul 23, 2019

corneliusweig commented May 7, 2019 •

edited

Loading

codecov-io commented May 8, 2019 •

edited by codecov bot

Loading

corneliusweig commented Jul 22, 2019 •

edited

Loading