Augment components with version and images from OCM component descriptors#87
Augment components with version and images from OCM component descriptors#87MartinWeindel wants to merge 14 commits intogardener:mainfrom
Conversation
…ub.com/go-sprout/sprout`
…rking-calico` and `networking-cilium`
f0204f2 to
5d7316f
Compare
5d7316f to
cbea833
Compare
LucaBernstein
left a comment
There was a problem hiding this comment.
Thank you very much, also for structuring the commits so well. A few preliminary comments from my side.
|
/assign |
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
timuthy
left a comment
There was a problem hiding this comment.
Thanks for integrating the OCM image processing into GLK 👏
| if err := os.MkdirAll(baseDir, 0700); err != nil { | ||
| return fmt.Errorf("failed to create output base directory %s: %w", baseDir, err) | ||
| } | ||
| if err := os.WriteFile(path.Join(baseDir, ".gitignore"), []byte("/*"), 0600); err != nil { |
There was a problem hiding this comment.
Is there a strong reason for not checking in the generated file? I'd say it's important to keep this state in the repository, so that glk generate can easily be invoked again w/o re-resolving the OCM image vector.
The debug files can still be written into a separate, ignored debug directory.
| // Name is the name of the resource. | ||
| Name string `json:"name"` | ||
| // Alias is an optional alias name of the resource. | ||
| Alias string `json:"alias,omitempty"` |
There was a problem hiding this comment.
Consider using a pointer if it's optional.
| // Resources is an optional list of values representing resources of the component. | ||
| Resources map[string]any `json:"resources,omitempty"` | ||
| // ImageVectorOverwrite is an optional image vector overwrite for the component. | ||
| ImageVectorOverwrite string `json:"imageVectorOverwrite,omitempty"` |
There was a problem hiding this comment.
Do we really have to use string here, can we be more precise?
| SourceRepository string `json:"sourceRepository"` | ||
| // Version is the version of the component. | ||
| Version string `json:"version"` | ||
| // Resources is an optional list of values representing resources of the component. |
There was a problem hiding this comment.
I think this needs further clarification, as the definition of a resource is not clear in this context. Please enhance the docstring by stating that a resource is usually an OCI image produced by a component.
| // Version is the version of the component. | ||
| Version string `json:"version"` | ||
| // Resources is an optional list of values representing resources of the component. | ||
| Resources map[string]any `json:"resources,omitempty"` |
There was a problem hiding this comment.
Do we really have to use any here, can we be more precise?
| "sigs.k8s.io/yaml" | ||
| ) | ||
|
|
||
| type ComponentVectorFactory func() (componentvector.ComponentVector, error) |
There was a problem hiding this comment.
| type ComponentVectorFactory func() (componentvector.ComponentVector, error) | |
| type BuildComponentVectorFn func() (componentvector.ComponentVector, error) |
| type ComponentVectorFactory func() (componentvector.ComponentVector, error) | ||
|
|
||
| // CreateComponentsVectorFile creates a components vector YAML file in the given filesystem. | ||
| func CreateComponentsVectorFile(fs afero.Afero, cvf ComponentVectorFactory) (string, error) { |
There was a problem hiding this comment.
| func CreateComponentsVectorFile(fs afero.Afero, cvf ComponentVectorFactory) (string, error) { | |
| func CreateComponentsVectorFile(fs afero.Afero, build BuildComponentVectorFn) (string, error) { |
| } | ||
|
|
||
| // ComponentVector creates a new ComponentVectorFactoryBuilder with the given name and version. | ||
| func ComponentVector(name, version string) *ComponentVectorFactoryBuilder { |
There was a problem hiding this comment.
| func ComponentVector(name, version string) *ComponentVectorFactoryBuilder { | |
| func NewComponentVectorBuilder(name, version string) *ComponentVectorFactoryBuilder { |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
LucaBernstein
left a comment
There was a problem hiding this comment.
Apart from what has already been remarked, I have some additional concerns about the imageVectorOverride templating, mainly w.r.t. indentations.
| {{- end }} | ||
| {{- if .imageVectorOverwrite }} | ||
| imageVectorOverwrite: | | ||
| {{ indent 10 .imageVectorOverwrite }} |
There was a problem hiding this comment.
| {{ indent 10 .imageVectorOverwrite }} | |
| {{ indent 10 .imageVectorOverwrite }} |
Seems to make a difference. Tried it out:
apiVersion: operator.gardener.cloud/v1alpha1
kind: Extension
metadata:
name: networking-calico
spec:
deployment:
extension:
values:
imageVectorOverwrite: |
images: // <-- wrong indentation!
- name: bird-exporter
repository: ghcr.io/czerwonk/bird_exporter
tag: v1.4.5There was a problem hiding this comment.
I'm not sure, though, why it doesn't show in the unit test with imageVectorOverwriteContent. 🤔
To me it seems that in the source ocm-components.yaml, there are <TAB> chars used instead of spaces.
→ Maybe we want to add some kind of strings.TrimSpace(...) to the image vector overwrite inserted?
| {{- end }} | ||
| {{- if .imageVectorOverwrite }} | ||
| imageVectorOverwrite: | | ||
| {{ indent 10 .imageVectorOverwrite }} |
There was a problem hiding this comment.
| {{ indent 10 .imageVectorOverwrite }} | |
| {{ indent 10 .imageVectorOverwrite }} |
| if cv.ImageVectorOverwrite != "" { | ||
| m["imageVectorOverwrite"] = cv.ImageVectorOverwrite | ||
| } | ||
| if cv.ComponentImageVectorOverwrites != "" { | ||
| m["componentImageVectorOverwrites"] = cv.ComponentImageVectorOverwrites | ||
| } |
There was a problem hiding this comment.
W.r.t. my previous comment: Maybe some pruning is required here?
Reproducibility of the indentation issues pointed out earlier would be nice though.
How to categorize this PR?
/area control-plane
/kind enhancement
What this PR does / why we need it:
The
resolve-ocm-componentscommand now writes aocm-components.yamlcontaining component vectors with resources, image overwrites, and component image overwrites.These additional information is then used during the
generate landscapecommand to update the helm chart, images references and overwrites in the templates.Additional processing of custom compoments with OCM component descriptors are supported, too. (See docs/usage/custom-ocm-components.md for details.)
Which issue(s) this PR fixes:
Fixes #67, #68, and #74
Special notes for your reviewer:
Release note: