Repository navigation
fix strategy: Merge for YAMLs in Glob sub-dirs - #383
jonasbadstuebner wants to merge 1 commit into
Conversation
|
If you have any input on my changes, let me know and I'll try to address it. Maybe there are edge cases I didn't think about. |
92e6b45 to
81f6921
Compare
| 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 |
There was a problem hiding this comment.
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.
Signed-off-by: Jonas Badstübner <jonas@jb.software>
81f6921 to
092ddbe
Compare
|
|
||
| if srcInfo.IsDir() { | ||
| return copyDirWithRoots(ctx, srcRoot, srcPath, stagingRoot, destPath, op.Exclude, excludeBasePath) | ||
| return copyDirWithRoots(ctx, op, srcRoot, srcPath, stagingRoot, destPath, op.Exclude, excludeBasePath) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yeah this is looking like a new strategy: MergeAll
There was a problem hiding this comment.
or MergeRecursively or something...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
3 participants
With this change, YAML files in sub-directories are merged as well.
Before only the YAML files in the root directory of the Glob pattern were merged, which made the test pass, but is not an intuitive behaviour.
fixes #382