fix(watch): exclude Dockerfile and compose files from initial sync - #14117
Conversation
Signed-off-by: hiroto.toyoda <hiroto.toyoda@dena.com>
glours
left a comment
glours
left a comment
There was a problem hiding this comment.
Hey @htoyoda18
Thanks for this contribution, can you address the few comments I made so we can merge it?
|
|
||
| // the Dockerfile and compose files drive the build/orchestration, not the | ||
| // application: never copy them into the container on initial sync | ||
| dockerFileIgnore, err := watch.NewDockerPatternMatcher("/", []string{"Dockerfile", "*compose*.y*ml"}) |
There was a problem hiding this comment.
*compose*.y*ml matches any basename containing "compose" plus a .y…ml suffix (e.g. my-compose-notes.yaml, recompose.yml), silently dropping unrelated files from the sync. Consider matching literal names from:
compose-go/v2/cli.DefaultFileNames/DefaultOverrideFileNames.
| // the Dockerfile and compose files drive the build/orchestration, not the | ||
| // application: never copy them into the container on initial sync |
There was a problem hiding this comment.
This restates the existing doc comment on initialSync a few lines above; consider dropping it or shortening it to just the new detail.
| // matcher nor EphemeralPathMatcher cover this. | ||
| func TestInitialSync_ExcludesDockerfileAndComposeFiles(t *testing.T) { | ||
| hostDir := t.TempDir() | ||
| for _, name := range []string{"Dockerfile", "compose.yaml", "compose.override.yml", "app.go"} { |
There was a problem hiding this comment.
compose.yaml and compose.override.yml both hit the same glob branch with an empty prefix, so the second file adds no coverage.
Swap it for docker-compose.yml instead, it exercises the wildcard matching a non-empty prefix, which thecurrent fixtures don't test.
|
|
||
| // the Dockerfile and compose files drive the build/orchestration, not the | ||
| // application: never copy them into the container on initial sync | ||
| dockerFileIgnore, err := watch.NewDockerPatternMatcher("/", []string{"Dockerfile", "*compose*.y*ml"}) |
There was a problem hiding this comment.
This only excludes the literal Dockerfile, so a custom-named one (build.dockerfile: Dockerfile.prod) still gets copied into the container, the same bug this PR fixes, just for non-default names.
Add service.Build.Dockerfile (nil-checked) to the pattern list.
…and basename Signed-off-by: hiroto.toyoda <hiroto.toyoda@dena.com>
|
@glours
While testing the last one, I found it didn't cover a Dockerfile in a |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
glours
left a comment
glours
left a comment
There was a problem hiding this comment.
Thanks for your contribution @htoyoda18
2 participants
What I did
initialSync's doc comment promises that the Dockerfile and compose files are never copied into the container duringdocker compose watch's initial sync — but the matcher enforcing that was dropped without replacement in a January 2025 refactor (#12469), leaving only a// FIXME .dockerignorecomment where it used to sit. Neither.dockerignore-derived ignores norEphemeralPathMatcher()(editor swap/temp files) cover this, so adevelop.watchtrigger whose path includes the project root would copyDockerfile/compose.yamlinto the running container on initial sync.Reintroduced a dedicated
dockerFileIgnorematcher (Dockerfile,*compose*.y*ml) insideinitialSync, matching the pre-refactor behavior.Scoped to
initialSynconly: the continuous watch loop's ignore (getWatchRules) never carried this exclusion, and — because it matches against full paths rather than basenames — would need a differently-anchored matcher to work there; that's a separate change if wanted.Related issue
N/A
(not mandatory) A picture of a cute animal, if possible in relation to what you did