WIP: Add OLM templater to support OLM's registry+v1 bundle format. #1376
Conversation
Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
|
Incidentally, the first commit of this PR bumps the kubernetes deps because the second commit's dependencies pull them in. I separated them out to help reduce noise. If maintainers are interested in the k8s 1.27 bump as a standalone PR, I can carve that out into a separate PR. |
|
Thank you for the PR @joelanford! I will try to take a look at it soon(ish).
🙏🏻 @rohitagg2020 is working on bumping the k8s libraries and controller runtime, so you should be able to rebase on those changes in future. |
|
|
||
| func Convert(in RegistryV1, installNamespace string, targetNamespaces []string) (*Plain, error) { | ||
| if installNamespace == "" { | ||
| installNamespace = in.CSV.Annotations["operatorframework.io/suggested-namespace"] |
There was a problem hiding this comment.
Nit for now, but functional change: We need to use the installNamespace from AppSpec.DefaultNamespace, instead of defaulting to <packageName>-system. (This would kind of bring us to a behaviour similar to OLM v0, where the install namespace is same as where the subscription is created?).
| // If the bundle supports AllNamespaces mode, default is all namespaces. Otherwise, if the | ||
| // bundle supports OwnNamespaces mode, default is the install namespace. If the bundle supports | ||
| // neither AllNamespaces nor OwnNamespace, TargetNamespaces must be specified. | ||
| TargetNamespaces []string `json:"targetNamespaces,omitempty" protobuf:"bytes,2,opt,name=targetNamespaces"` |
There was a problem hiding this comment.
@joelanford what is the value of having TargetNamespaces here? Can't we get it directly from csv's install modes? In rukpak's implementation, we always pass an empty slice for target namespaces when converting from plain to registry: https://github.com/operator-framework/rukpak/blob/0e327c385ca5488da4c2eca95a68d385c7e08a5f/internal/convert/registryv1.go#L172 (and this was assuming we won't support multi-ns bundles).
Are we looking to support multi namespace mode in v1, and accept that option from Operator API which would then be passed here?
Follow up thought on that, if that's the case, we should handle it for the Helm and plain bundles also somehow. And probably App is the right place to expose it? Though we would not want App API to be showcased as "operator" specific.
| PackageName string `json:"operators.operatorframework.io.bundle.package.v1" yaml:"operators.operatorframework.io.bundle.package.v1"` | ||
|
|
||
| // Channels are a comma separated list of the declared channels for the bundle, ala `stable` or `alpha`. | ||
| Channels string `json:"operators.operatorframework.io.bundle.channels.v1" yaml:"operators.operatorframework.io.bundle.channels.v1"` |
There was a problem hiding this comment.
Hmm.. these concepts are to be supported in other types of bundles too. The same question from the previous thought, what is the right place to expose this if it is generic.
What this PR does / why we need it:
This PR is a proof of concept for #1369.
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?
Additional Notes for your reviewer:
At this stage, this PR is mostly gluing some existing OLM code together and transplanting some of it here. My main goal is to get feedback about the
AppAPI changes and to see if the overall approach is something that the Carvel community thinks is on the right track. I'm not really looking (yet) for a review of the underlying "templating" code, its structure, or its repo location (don't let that stop you though! I'm open to any and all feedback)Review Checklist:
a link to that PR
change
Additional documentation e.g., Proposal, usage docs, etc.: