feat: add a YAML Merger PromotionStep #4634
Conversation
✅ Deploy Preview for docs-kargo-io ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
prune998
force-pushed
the
prune/yaml-merger
branch
from
89bbf3d to
f13147f
Compare
prune998
changed the title
prune998
force-pushed
the
prune/yaml-merger
branch
3 times, most recently
from
bde39d7 to
e3bed04
Compare
|
Any way to get someone trigger the CI build ? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4634 +/- ##
==========================================
+ Coverage 53.29% 53.35% +0.05%
==========================================
Files 371 372 +1
Lines 32456 32559 +103
==========================================
+ Hits 17299 17371 +72
- Misses 14264 14289 +25
- Partials 893 899 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
prune998
force-pushed
the
prune/yaml-merger
branch
from
3268947 to
e36bfe8
Compare
|
thanks for the review @fuskovic. Everything should be resolved + rebased now. |
|
Anyone to review this ? |
|
@prune998 I know this isn't what anyone wants to hear, but our priorities are elsewhere right now and there may be a bit of a wait for a thorough review. Some discussion may also be in order. I see this is meant to address #4766, but no maintainer triaged that feature request or moved it out of proposal status. It's a gamble putting the effort into a feature without those conversations happening first. In all probability, this is a worthwhile feature and it will be reviewed eventually and probably merged, but some patience may be required. If it makes you feel any better, I have PRs open to some other projects that are old enough to be starting kindergarten. Our backlog isn't that bad, thankfully. |
|
Small addendum... there are some notoriously well-known gotchas when merging YAML, so this isn't a review I'd rush through and that contributes to the above. |
|
Thanks for the comment @krancour . I understand you're busy with your customers features. What you could do, would be
This way, your team will not be much impacted, the features is released, and you have a "feature maintainer". This is just a proposition and you can refuse it. I'll wait, or keep using my forked version... but it will be a miss :) Side note on YAML merging: yeah, there's a ton of gotchas... this is why I used a "well known" library to do it and not try to re-implement it myself. The library I used is also opinionated. But it is intended for K8s yaml style. I also tested others, with varying results... Anyway, they all have the same limitations (biggest one being no merge of lists, latest list takes precedence), like Helm does, so people knowing Helm will not be disturbed. |
Signed-off-by: Prune <prune@lecentre.net>
Signed-off-by: Prune <prune@lecentre.net>
Signed-off-by: Prune <prune@lecentre.net>
Signed-off-by: Prune <prune@lecentre.net>
Signed-off-by: Prune <prune@lecentre.net>
Signed-off-by: Prune <prune@lecentre.net>
Signed-off-by: Prune <prune@lecentre.net>
Signed-off-by: Prune <prune@lecentre.net>
Signed-off-by: Prune <prune@lecentre.net>
Signed-off-by: Prune <prune@lecentre.net>
prune998
force-pushed
the
prune/yaml-merger
branch
from
c285a55 to
9701c2d
Compare
| // sanity check | ||
| if len(cfg.InFiles) == 0 || cfg.OutFile == "" { | ||
| return promotion.StepResult{Status: kargoapi.PromotionStepStatusErrored}, fmt.Errorf("inFiles and OutFile must not be empty") | ||
| } |
There was a problem hiding this comment.
Wouldn't validation against the schema already have caught either of these issues?
| if cfg.IgnoreMissingFiles { | ||
| continue | ||
| } | ||
| return promotion.StepResult{Status: kargoapi.PromotionStepStatusErrored}, fmt.Errorf("input file not found: %s", inFile) |
There was a problem hiding this comment.
We don't actually know that "file not found" was the problem because we haven't checked os.IsNotExist(err). I think we ought to do so we don't produce an error message indicating "file not found" when the problem may have been something else entirely.
| // MergeYAMLFiles merges a list of YAML files. | ||
| func MergeYAMLFiles(inputPaths []string, outputPath string) error { |
There was a problem hiding this comment.
We would usually prefer not to repeat context that can be inferred from the package name. This will be used as yaml.MergeYAMLFiles(), which sounds redundant. yaml.MergeFiles() is no less clear.
| // MergeYAMLFiles merges a list of YAML files. | |
| func MergeYAMLFiles(inputPaths []string, outputPath string) error { | |
| // MergeFiles merges a list of YAML files. | |
| func MergeFiles(inputPaths []string, outputPath string) error { |
|
|
||
| mergedNode, err = merge2.Merge(patchNode, mergedNode, kyaml.MergeOptions{ListIncreaseDirection: 1}) | ||
| if err != nil { | ||
| return fmt.Errorf("error merging node at index %d: %w", i, err) |
There was a problem hiding this comment.
This error message would probably be more useful if instead of talking about nodes it included the path of the file that couldn't be merged.
This PR add a new Promotion
Stepthat caan merge multiple YAML files into a single file.This is useful in the scenario where, for example, multiple promotions (ex: one per cluster) merges content in the same repo/branch.
Then Helm will pick up the values from multiple locations, but updating
globalvalues could impact many clusters at the same time.With this step, we are able to pre-merge
globalvalues with more specific values and only commit one singlevalues.yamlfile for each specific cluster. No moreglobals.