[Add] Support for other Install strategies #772
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #772 +/- ##
==========================================
+ Coverage 20.62% 20.73% +0.11%
==========================================
Files 14 14
Lines 1086 1085 -1
==========================================
+ Hits 224 225 +1
+ Misses 812 811 -1
+ Partials 50 49 -1 ☔ View full report in Codecov by Sentry. |
varshaprasad96
changed the title
varshaprasad96
force-pushed
the
support/watchnamespaces
branch
from
6fe21a6
to
3b534fa
Compare
This PR removes the requirement that AllNamespace mode should be supported always in a registry+v1 format. It does so by: 1. Adding a field to specify watch namespaces in Bundle Template. 2. While converting from registryV1 to plain bundle, we now generate roles for all specified targetNamespaces which are being watched. Signed-off-by: Varsha Prasad Narsing <[email protected]>
varshaprasad96
force-pushed
the
support/watchnamespaces
branch
from
3b534fa
to
2a7a6c7
Compare
| @@ -189,9 +189,6 @@ func Convert(in RegistryV1, installNamespace string, targetNamespaces []string) | |||
| supportedInstallModes.Insert(string(im.Type)) | |||
| } | |||
| } | |||
| if !supportedInstallModes.Has(string(v1alpha1.InstallModeTypeAllNamespaces)) { | |||
| return nil, fmt.Errorf("AllNamespace install mode must be enabled") | |||
| } | |||
| if targetNamespaces == nil { | |||
There was a problem hiding this comment.
Would you mind changing this to check for len=0 instead please?
| func Simple(in RegistryV1, watchNamespaces []string) (*Plain, error) { | ||
| return Convert(in, "", watchNamespaces) | ||
| } |
There was a problem hiding this comment.
I think I'd just drop the Simple function at this point.
| // +kubebuilder:validation:Optional | ||
| // watchNamespaces indicates which namespaces the operator should watch. | ||
| WatchNamespaces []string `json:"watchNamespaces,omitempty"` |
There was a problem hiding this comment.
Hmm. I had imagined that we'd put this on the BundleDeployment, not the Bundle. But the wrinkle with needing to generate extra RBAC during the conversion means this is essential information in that step.
One possible solution here would be to make a registry BundleDeployment provisioner and move the conversion logic there. If we did that, we could probably make the Bundle provisioner super generic and reusable for registry/plain/helm/etc. Basically it would just be a "give me the filesystem with no format-specific validation" provisioner. And then the BD would be the place for registry/plain/helm to have separate provisioners and do whatever specific validation was necessary.
From an API design standpoint, I argue that configuration of the templating of the bundle belongs in the BundleDeployment.
All of this highlights the weird dichotomy between Bundle and BundleDeployment, and I think it's another good reason to move to a single API for "source/template/apply" like the Carvel App API. I think moving toward a super-generic Bundle provisioner gets us more aligned with that concept anyway, so perhaps that would be a good step in that direction?
What do you think?
There was a problem hiding this comment.
I missed this was on Bundle and not BundleDeployment. +100 to getting rid of Bundle. @varshaprasad96 if you're around this week and want help, please let me know.
There was a problem hiding this comment.
@joelanford @ncdc fair. The reason for having this on template, was to be able to pass this to bundle provisioner which handles the conversion. If we move that logic to Bundle, there doesn't seem to be much value in having a separate API.
I think it's another good reason to move to a single API for "source/template/apply" like the Carvel App API
Removing Bundle would change the BundleDeployment API, we would need an alpha2? Nevertheless, I'll get a draft PR ready, and we can discuss the specifics of the API there.
|
Closing in favour of #802 |
This PR removes the requirement that AllNamespace mode should be supported always in a registry+v1 format.
It does so by: