Skip to content

fix strategy: Merge for YAMLs in Glob sub-dirs #383

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.

Sign up for GitHub

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

Open
jonasbadstuebner wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix strategy: Merge for YAMLs in Glob sub-dirs #383

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion internal/builder/builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,7 @@ func copyFileWithRoots(ctx context.Context,
}

if srcInfo.IsDir() {
return copyDirWithRoots(ctx, srcRoot, srcPath, stagingRoot, destPath, op.Exclude, excludeBasePath)
return copyDirWithRoots(ctx, op, srcRoot, srcPath, stagingRoot, destPath, op.Exclude, excludeBasePath)

@stefanprodan stefanprodan Oct 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This also changes copies of a plain directory path, not just globs. from: "@git/overrides/" with strategy: Merge goes through applySingleDirectoryCopy → copyFileWithRoots → copyDirWithRoots, so it now merges too. Which means:

  • Any colliding file that isn't YAML now fails the build with cannot unmarshal YAML document. Examples: .md files, Helm templates/*.tpl, or templates with {{ }}. Before, these were silently overwritten.
  • Colliding YAML files are re-marshaled through loadYAML. That folds multi-document files into one map (merge.go:41-54). Running Merge on a whole directory of Kubernetes manifests would corrupt them.

At least we should say in the docs that Merge applies recursively and that every colliding file must be single-document YAML/JSON.

@stefanprodan stefanprodan Oct 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If someone has Merge set for a dir, after this change we'll corrupt their YAMLs. We are still in beta with the API, but still this is a major breaking change, not sure if a doc entry is acceptable. cc @matheuscscp

@jonasbadstuebner jonasbadstuebner Oct 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thank you for looking into my PR!
Could you provide test cases where you think they should succeed and they don't with my changes applied?
It would help me a lot to verify that the changes I make don't go against the behaviour you expect from source-watcher.

Before, these were silently overwritten.

Should we check for file endings to be .yaml/.yml? And only merge those and copy the others?

Running Merge on a whole directory of Kubernetes manifests would corrupt them.

Only if they have multiple documents inside, I suppose.

If someone has Merge set for a dir, after this change we'll corrupt their YAMLs.

If someone has Merge set for a dir, Merge currently does nothing, right? That's why I thought that this can't be the intended behaviour.
We could also bump the API (source.extensions.fluxcd.io/v1beta2?) and only apply the new behaviour when the version is v1beta2?

@stefanprodan stefanprodan Oct 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would skip merge using probes in this order:

  • check .yaml / .yml
  • parse the YAML with sig/yaml, skip on error (hopefully this will fail on Helm templates so we do not corrupt them)
  • check for multi-doc, skip if len(doc) > 1

Log the reason for skipping.

@matheuscscp matheuscscp Oct 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah this is looking like a new strategy: MergeAll

@matheuscscp matheuscscp Oct 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

or MergeRecursively or something...

@jonasbadstuebner jonasbadstuebner Oct 2, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Where I'm coming from is that the glob pattern from your test (@source2/**) matches all files in that folder recursively so it is most intuitive that it will take them one by one and merge them onto the ones in the source.

If my PR would be a change that introduces a new strategy, the Merge strategy with its current behaviour is confusing, because yaml files in the root directory get merged but in the child directories they don't.
The ...Recursively part of the suggested name for a new strategy is already expressed by the glob pattern (**). And ...All would be just as confusing because it's only merging Yaml files anyway.

@matheuscscp matheuscscp Oct 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmmm... Yeah.... A new strategy would make this very confusing indeed. Maybe we can ship this by bumping to v1beta2? Maybe the controller can check the apiVersion used by the object? Not sure if this is possible, but if it was then we could have different behaviors for v1beta1 and v1beta2 without a breaking change right now. We will remove v1beta1 one day, and that will obviously be a breaking change that we will accept. So if we can branch the code between v1beta1 and v1beta2 then we can ship this fix and make it opt-in for now.

@stefanprodan stefanprodan Oct 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Agreed, the strategy is how a file in processed. Adding Recursively/All makes things very confusing, since the pattern dictates if the operation is recursive or not.

}

if shouldMergeFile(op, stagingRoot, destPath) {
Expand Down Expand Up @@ -591,6 +591,7 @@ func mergeFileWithRoots(ctx context.Context,
// copyDirWithRoots copies a directory recursively using os.Root,
// skipping files and sub-dirs matching exclude patterns.
func copyDirWithRoots(ctx context.Context,
op swapi.CopyOperation,
srcRoot *os.Root,
srcPath string,
stagingRoot *os.Root,
Expand Down Expand Up @@ -633,6 +634,10 @@ func copyDirWithRoots(ctx context.Context,
return createDirRecursive(stagingRoot, destFilePath)
}

if shouldMergeFile(op, stagingRoot, destFilePath) {
return mergeFileWithRoots(ctx, srcRoot, path, stagingRoot, destFilePath)
}

return copyRegularFileWithRoots(ctx, srcRoot, path, stagingRoot, destFilePath)
})
}
Expand Down
30 changes: 27 additions & 3 deletions internal/builder/merge_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ c:
source2Dir := filepath.Join(tmpDir, "source2")
workspaceDir := filepath.Join(tmpDir, "workspace")

setupDirs(t, source1Dir, source2Dir, workspaceDir)
setupDirs(t, filepath.Join(source1Dir, "config"), filepath.Join(source2Dir, "config"), workspaceDir)

// Create first source with base config
createFile(t, source1Dir, "config1.yaml", `
Expand All @@ -186,11 +186,17 @@ region: us-west-1
createFile(t, source1Dir, "config2.yaml", `
version: 1.0.0
image: my-app:latest
`)
createFile(t, filepath.Join(source1Dir, "config"), "config3.yaml", `
version: 1.0.0
image: my-app:latest
`)

// Create second source with overlay config
createFile(t, source2Dir, "config1.yaml", "env: prod") // This should overwrite the env
createFile(t, source2Dir, "config2.yaml", "replicas: 5") // This should add a new field
createFile(t, source2Dir, "config1.yaml", "env: prod") // This should overwrite the env
createFile(t, source2Dir, "config2.yaml", "replicas: 5") // This should add a new field in the root directory of the glob pattern
createFile(t, filepath.Join(source2Dir, "config"), "config3.yaml", "replicas: 10") // This should add a new field in a subdirectory matched by the glob pattern
createFile(t, filepath.Join(source2Dir, "config"), "config4.yaml", "content: hello-world") // This should add a new file

@jonasbadstuebner jonasbadstuebner Sep 18, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I added this test because the first iteration of my PR had a bug where if the target file would not exist, it would still have tried to merge the source file into it which lead to an error.
I think one more test case won't hurt.


spec := &swapi.OutputArtifact{
Name: "yaml-to-yaml-dir-merge",
Expand Down Expand Up @@ -222,6 +228,8 @@ image: my-app:latest
stagingDir := filepath.Join(workspaceDir, "yaml-to-yaml-dir-merge")
config1Path := filepath.Join(stagingDir, "config1.yaml")
config2Path := filepath.Join(stagingDir, "config2.yaml")
config3Path := filepath.Join(stagingDir, "config", "config3.yaml")
config4Path := filepath.Join(stagingDir, "config", "config4.yaml")

config1Content, err := os.ReadFile(config1Path)
g.Expect(err).ToNot(HaveOccurred())
Expand All @@ -231,6 +239,14 @@ image: my-app:latest
g.Expect(err).ToNot(HaveOccurred())
g.Expect(config2Content).ToNot(BeEmpty())

config3Content, err := os.ReadFile(config3Path)
g.Expect(err).ToNot(HaveOccurred())
g.Expect(config3Content).ToNot(BeEmpty())

config4Content, err := os.ReadFile(config4Path)
g.Expect(err).ToNot(HaveOccurred())
g.Expect(config4Content).ToNot(BeEmpty())

// Verify the merged YAML contains expected content
g.Expect(config1Content).To(MatchYAML(`
env: prod
Expand All @@ -240,6 +256,14 @@ region: us-west-1
image: my-app:latest
replicas: 5
version: 1.0.0
`))
g.Expect(config3Content).To(MatchYAML(`
image: my-app:latest
replicas: 10
version: 1.0.0
`))
g.Expect(config4Content).To(MatchYAML(`
content: hello-world
`))
},
},
Expand Down