Improve syntax for artifact.sync config [1/3] by corneliusweig · Pull Request #1847 · GoogleContainerTools/skaffold

corneliusweig · 2019-03-21T00:39:07Z

Currently, the pipeline config is a map of local glob pattern to destination directory. This scheme has been extended with some magic sequences, making it difficult to understand. This commit converts the sync map into a list of sync rules. All sync rules are considered to determine the destination paths.

This is the first step of sync improvements as detailed out in the design proposal #1844.

Extracted from #1812
Close #1180

CC design shepherd @tejal29

codecov-io · 2019-04-03T19:36:43Z

Codecov Report

Merging #1847 into master will increase coverage by 0.09%.
The diff coverage is 85.13%.

@@            Coverage Diff             @@
##           master    #1847      +/-   ##
==========================================
+ Coverage   56.18%   56.27%   +0.09%     
==========================================
  Files         180      180              
  Lines        7771     7781      +10     
==========================================
+ Hits         4366     4379      +13     
+ Misses       2988     2987       -1     
+ Partials      417      415       -2

Impacted Files	Coverage Δ
pkg/skaffold/schema/latest/config.go	`100% <ø> (ø)`	⬆️
pkg/skaffold/sync/kubectl/kubectl.go	`9.67% <0%> (ø)`	⬆️
pkg/skaffold/util/tar.go	`35.71% <0%> (-0.44%)`	⬇️
pkg/skaffold/schema/validation/validation.go	`96.87% <100%> (+0.57%)`	⬆️
pkg/skaffold/schema/v1beta9/upgrade.go	`84.9% <100%> (+18.23%)`	⬆️
pkg/skaffold/sync/sync.go	`70.32% <96%> (-4.06%)`	⬇️

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 63f353f...4b35761. Read the comment docs.

corneliusweig · 2019-04-17T10:47:40Z

@tejal29 As you were shepherd of the design proposal, you might also want to take a look at this :)

tejal29 · 2019-04-17T17:31:32Z

Thanks @corneliusweig I was waiting for Design proposal to merge in.

tejal29

LGTM! overall.

tejal29 · 2019-05-08T16:48:51Z

docs/content/en/samples/filesync/filesync.yaml

+            dest: /etc
+          - src: 'static-html/*.html'
+            dest: static
+          - src: '**/*.png'


A comment on what the intended behavior is

I've added some comments. Please have a look if this is what you intended. I'm not sure whether I like those comments, though. After all, each rule is explained further below. But I trust you have a better feeling for what's appropriate here.

tejal29 · 2019-05-08T16:51:10Z

pkg/skaffold/schema/validation/validation_test.go

+						{
+							Src:   "src/**/*.js",
+							Dest:  ".",
+							Strip: "src/",


Can we add a test case where ?

{ Src: "srcsomeother/**/*.js", Dest: ".", Strip: "src", }

corneliusweig · 2019-05-08T17:45:15Z

I will take a look as soon as get to work today.

Hey @tejal29 , thanks for giving this such high priority! Very much appreciated!

Signed-off-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>

corneliusweig · 2019-05-08T17:57:32Z

@tejal29 nits done. PTAL

priyawadhwa

LGTM, just a couple docs/warning suggestions

priyawadhwa · 2019-05-08T18:06:02Z

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

+A manual sync rule must specify the `src` and `dest` field.
+The `src` field is a glob pattern to match files relative to the artifact _context_ directory, which may contain `**` to match nested files.
+The `dest` field is the destination location in the container.
+It may be absolute or relative, in which case files are put below the container's `WORKDIR`.


Just for clarity, I would rephrase this as Destination paths may be absolute or relative. If the destination is a relative path, an absolute path will be inferred by prepending the path with the container's WORKDIR

priyawadhwa · 2019-05-08T18:07:34Z

pkg/skaffold/schema/v1beta9/upgrade.go

+)
+
+const (
+	incompatibleSyncWarning = `the semantics of the sync rules has changed, the folder structure is not flattened anymore but preserved, the likely impacted patterns in your skaffold yaml are: %s`


Might be nice to provide a link to the docs here so people can easily find the new sync rules

Signed-off-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>

corneliusweig · 2019-05-08T18:52:03Z

@priyawadhwa Thanks for your valuable input! PTAL

tejal29 · 2019-05-09T21:59:24Z

Thanks @corneliusweig, i have brought this to this week's release master's attention :)
They will merge it in if things look good.
From my side #LGTM

corneliusweig · 2019-05-09T22:00:58Z

@tejal29 Unless I completely misunderstood you, you hit the wrong button ;)

balopat · 2019-05-09T22:02:23Z

haha, I think it was accidental :D

container-tools-bot · 2019-05-09T22:08:58Z

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

balopat

LGTM, thank you for your hard work and patience on this.
One thought for the next PRs: maybe we could name our new map[string][]string as syncMap

corneliusweig · 2019-05-09T22:41:16Z

@balopat Oh, it was a pleasure (most of the time :)

About syncMap, do you think about a local type alias in the sync package, or more on the global scope alongside interface Builder? Anyway, you're right, there really should be a type alias.

balopat · 2019-05-09T23:06:29Z

I'm rerunning kokoro - this seems to be a flake...

time="2019-05-09T22:32:42Z" level=info msg="Ran in 6.011332858s"
--- FAIL: TestGetStateRPC (7.12s)
    rpc_test.go:286: waiting for connection...
    rpc_test.go:246: skaffold build or deploy not complete. state: buildState:<artifacts:<key:"gcr.io/k8s-skaffold/test-dev" value:"In Progress" > > deployState:<status:"Not Started" >

if it is, I'll open a bug.

balopat · 2019-05-09T23:09:05Z

@balopat Oh, it was a pleasure (most of the time :)

About syncMap, do you think about a local type alias in the sync package, or more on the global scope alongside interface Builder? Anyway, you're right, there really should be a type alias.

I'd keep it in sync - seems like a local concept for now. we should pull it up to a wider scope only if required.

corneliusweig · 2019-05-09T23:19:22Z

What a thriller, these integration tests...

balopat · 2019-05-09T23:27:31Z

#2100 for the flake

) * Improved pipeline config for artifact.sync Currently, the pipeline config is a map of local glob pattern to destination directory. This scheme has been extended with some magic sequences, making it difficult to understand. This commit converts the sync map into a list of sync rules. All sync rules are consulted to determine the destination paths. This prepares for further changes to the sync logic. Also see GoogleContainerTools#1844 * Update sync spec config according to design proposal - Wrap sync-rules under key 'sync.manual' - Config changes: - from -> src - to -> dest - flatten option removed and warn during schema upgrade if an incompatible pattern is migrated to the new schema version * Add validation for sync rules * Migrate sync config to new schema version * Update filesync documentation Extract example into sample snippet in order to have it tested. * Run validation on doc examples * Review comments: add test case and clarify example in docs * Improve doc and error message wording Signed-off-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>

googlebot added the cla: yes label Mar 21, 2019

corneliusweig changed the title ~~Improve pipeline config for artifact.sync~~ WIP Improve pipeline config for artifact.sync Mar 21, 2019

This was referenced Mar 21, 2019

WIP Sync: infer destination from Dockerfile #1812

Closed

Sync: subtree sync needs documentation #1837

Closed

corneliusweig force-pushed the sync-config branch 2 times, most recently from b4b1960 to 7341dbe Compare April 3, 2019 18:56

corneliusweig changed the title ~~WIP Improve pipeline config for artifact.sync~~ Improve pipeline config for artifact.sync Apr 3, 2019

corneliusweig changed the title ~~Improve pipeline config for artifact.sync~~ Improved pipeline config for artifact.sync Apr 3, 2019

corneliusweig marked this pull request as ready for review April 3, 2019 18:59

corneliusweig requested review from balopat, dgageot, nkubala, priyawadhwa and r2d4 as code owners April 3, 2019 18:59

corneliusweig force-pushed the sync-config branch 2 times, most recently from 1a30aff to f6808e9 Compare April 3, 2019 19:47

corneliusweig changed the title ~~Improved pipeline config for artifact.sync~~ WIP Improved pipeline config for artifact.sync Apr 4, 2019

corneliusweig force-pushed the sync-config branch 3 times, most recently from ffa083a to 3f0a875 Compare April 8, 2019 11:33

corneliusweig changed the title ~~WIP Improved pipeline config for artifact.sync~~ WIP Improve syntax for artifact.sync config Apr 8, 2019

corneliusweig force-pushed the sync-config branch 2 times, most recently from 0ca1bbb to 43ec0e6 Compare April 8, 2019 22:14

corneliusweig changed the title ~~WIP Improve syntax for artifact.sync config~~ Improve syntax for artifact.sync config Apr 9, 2019

corneliusweig force-pushed the sync-config branch from 43ec0e6 to 3a0ffc0 Compare April 10, 2019 20:00

corneliusweig mentioned this pull request Apr 13, 2019

Fix syncing for Jib #1926

Merged

corneliusweig force-pushed the sync-config branch from 3a0ffc0 to 8da745a Compare April 16, 2019 20:38

tejal29 reviewed May 8, 2019

View reviewed changes

Merge remote-tracking branch 'origin/master' into sync-config

2899a52

Review comments: add test case and clarify example in docs

adc783b

Signed-off-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>

priyawadhwa reviewed May 8, 2019

View reviewed changes

Improve doc and error message wording

4b35761

Signed-off-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>

tejal29 closed this May 9, 2019

balopat reopened this May 9, 2019

balopat added !! config-change !! docs-modifications runs the docs preview service on the given PR labels May 9, 2019

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

balopat approved these changes May 9, 2019

View reviewed changes

balopat added the kokoro:run runs the kokoro jobs on a PR label May 9, 2019

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

balopat merged commit e0dd2b9 into GoogleContainerTools:master May 9, 2019

corneliusweig deleted the sync-config branch May 9, 2019 23:27

corneliusweig mentioned this pull request Jul 5, 2019

Correctly migrate sync config in profiles #2415

Merged

Conversation

corneliusweig commented Mar 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Apr 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

corneliusweig commented Apr 17, 2019

Uh oh!

tejal29 commented Apr 17, 2019

Uh oh!

tejal29 left a comment

Choose a reason for hiding this comment

Uh oh!

tejal29 May 8, 2019

Choose a reason for hiding this comment

Uh oh!

corneliusweig May 8, 2019

Choose a reason for hiding this comment

Uh oh!

tejal29 May 8, 2019

Choose a reason for hiding this comment

Uh oh!

corneliusweig May 8, 2019

Choose a reason for hiding this comment

Uh oh!

corneliusweig commented May 8, 2019

Uh oh!

corneliusweig commented May 8, 2019

Uh oh!

priyawadhwa left a comment

Choose a reason for hiding this comment

Uh oh!

priyawadhwa May 8, 2019

Choose a reason for hiding this comment

Uh oh!

priyawadhwa May 8, 2019

Choose a reason for hiding this comment

Uh oh!

corneliusweig commented May 8, 2019

Uh oh!

tejal29 commented May 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

corneliusweig commented May 9, 2019

Uh oh!

balopat commented May 9, 2019

Uh oh!

container-tools-bot commented May 9, 2019

Uh oh!

balopat left a comment

Choose a reason for hiding this comment

Uh oh!

corneliusweig commented May 9, 2019

Uh oh!

balopat commented May 9, 2019

Uh oh!

balopat commented May 9, 2019

Uh oh!

corneliusweig commented May 9, 2019

Uh oh!

balopat commented May 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

corneliusweig commented Mar 21, 2019 •

edited

Loading

codecov-io commented Apr 3, 2019 •

edited

Loading

tejal29 commented May 9, 2019 •

edited

Loading