-
-
Notifications
You must be signed in to change notification settings - Fork 98
feat: add partial manifest #5508
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces support for partial manifests in Updatecli by allowing manifest files starting with an underscore (e.g. _source.yaml) to be merged into the main manifest from the same directory.
- Refactored file path sanitization to return both manifest and partial file paths.
- Updated the registry and configuration loaders to include partial files when processing manifests.
- Added tests and e2e examples to validate the new partial manifest functionality.
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
pkg/core/engine/utils.go | Refactored file path sanitization to separate regular and partial manifests. |
pkg/core/engine/testdata/partialOneManifest/updatecli.yaml | Added manifest test file for partial manifest integration. |
pkg/core/engine/testdata/partialOneManifest/_source.yaml | Added a partial manifest test file. |
pkg/core/engine/registry.go | Updated registry push to include partial file paths. |
pkg/core/engine/configuration_test.go | Extended tests to cover partial manifest scenarios and error formatting. |
pkg/core/engine/configuration.go | Adjusted configuration loading to process partial files and update error messages. |
pkg/core/config/main.go | Modified manifest loading to combine partial file content with the main manifest. |
e2e/updatecli.d/success.d/partial/stable/_source.yaml | Added partial file for e2e testing of stable sources. |
e2e/updatecli.d/success.d/partial/jenkins.yaml | Added jenkins pipeline manifest using partial manifests. |
e2e/updatecli.d/success.d/partial/_source.yaml | Added supplemental partial file for e2e testing. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
In case of a mistake, Updatecli returns an error similar to
It's not easy to provide better error message since the manifest need to be rendered as a whole before validiting it |
I like the idea of automatically loading any That said, I think it would offer even greater composability to support a dedicated keyword in the manifest to explicitly declare which files to load. For instance, an This would allow each pipeline to declare exactly which partial configs it depends on, resulting in a more DRY and modular setup. I realize this overlaps somewhat with the Updatecli policy feature, which supports storing versioned config fragments in an OCI repository. Still, I believe this approach would be more straightforward for users, as it avoids external dependencies and works with just a Git repository. Of course, what I'm proposing here could be moved into a separate task. The current PR could be merged as-is since it's a solid first step toward more composable updatecli pipeline definitions. |
In theory, I agree with you, in practice, I have a chicken-and-egg problem. Because of the golang templating we would first need to render the manifest in order to be sure that the "imports" settings is correct and then we could re-render the manifest a second time with the partial files listed in the import section. I fear that it could lead to confusion |
I just can't see a way to avoid this technical limitation as it's a chicken and egg problem |
Fix #430
As discussed in #5461 (comment)
I propose to introduce partial manifest, similar to how the Helm project is doing.
A partial manifest is a specific type of Updatecli manifest starting with
_
such as_scm.yaml
that is available toall the Updatecli manifest within the same directory.
Test
To test this pull request, you can run the following commands:
go build -o bin/updatecli . ./bin/updatecli manifest show --config e2e/updatecli.d/success.d/partial
You will notice that it only generate one Updatecli manifest by combining the files
_source.yaml
andjenkins.yaml
The file
stable/_source.yaml
is correctly ignore as being a partial fileAdditional Information
Checklist
Tradeoff
In the current implementation all the files are concatenated together so any YAML file starting with
---
will be considered as a different YAML document which may lead to some confusion on a large project.By design, partial manifest are only available within the same directory
Potential improvement
/