Uh oh!
There was an error while loading. Please reload this page.
✨ Refactor rukpak convert (now with testing!) - #1893
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| Template: corev1.PodTemplateSpec{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Annotations: map[string]string{ | ||
| "csv": "annotation", |
There was a problem hiding this comment.
I always have to refresh my memory on this question: CSV annotations are copied into the pod annotations, but not the deployment annotations? That may very well be the case, it's just that it never obvious if there is anything that motivates this.
There was a problem hiding this comment.
I had the same questions in my mind. This is probably a game of telephone, but I just wanted to make sure we had what we currently have in the convert function. I don't know the motivation behind this =S
688bd22 to
7aaadf9CompareCodecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #1893 +/- ##
==========================================
+ Coverage 65.87% 66.37% +0.50%
==========================================
Files 71 75 +4 Lines 6189 6326 +137 ==========================================
+ Hits 4077 4199 +122 - Misses 1850 1864 +14 - Partials 262 263 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
7aaadf9 to
0589b6cCompare3ac6014 to
7b25a77Compare7b25a77 to
3adc8ddCompareUh oh!
There was an error while loading. Please reload this page.
3adc8dd to
d6858e6Compareeea8fba to
9585806Comparef63c8e6 to
744a674Compare
anik120
left a comment
There was a problem hiding this comment.
I realized that while testing is being added to test the convert functionalities, the testing is exclusively on the new components (ResourceGenerators, BundleRenders etc).
Maybe a way to break this PR would have to be add tests in the first PR that tests the conversion exhaustively.
Then the new PR would refractor and add the new components, with minimal changes to the tests, while the pieces being called would obviously need to change, the behavior would not change in the existing tests.
That way we can be sure that we're not regressing on any functionality while refactoring and introducing the new components.
| var PlainConverter = Converter{ | ||
| BundleRenderer: render.BundleRenderer{ | ||
| BundleValidator: validators.RegistryV1BundleValidator, | ||
| ResourceGenerators: []render.ResourceGenerator{ | ||
| generators.BundleCSVRBACResourceGenerator.ResourceGenerator(), | ||
| generators.BundleCRDGenerator, | ||
| generators.BundleAdditionalResourcesGenerator, | ||
| generators.BundleCSVDeploymentGenerator, | ||
| }, | ||
| }, | ||
| } | ||
| type Converter struct { | ||
| BundleValidator BundleValidator | ||
| render.BundleRenderer | ||
| } | ||
There was a problem hiding this comment.
This feels a bit weird to me. I get the feeling that these changes are incorporating something new, to something existing. But do we have to keep the existing layout?
What are your thoughts on the following:
var PlainConverter = render.BundleRenderer func (r BundleRenderer) Convert(rv1 RegistryV1, installNamespace string, targetNamespaces []string) (*Plain, error) {
.
.
}
PlainConverter.Convert(...)
And then take that one step further:
var RegistryV1ToPlainBundleRenderer = render.BundleRenderer RegistryV1ToPlainBundleRenderer.Convert(...)
func (r BundleRenderer) Convert(rv1 RegistryV1, installNamespace string, targetNamespaces []string) (*Plain, error) {
.
.
}
ie get rid of Converter totally, with the Convert function defined in the render package as a BundleRenderer component.
Isn't the PR essentially replacing Converter with Renderer?
There was a problem hiding this comment.
Eventually I'd like to do away with the convert package all together in favor of the renderer. I just didn't want to also carry over all the logic checking the install and target namespaces, and validating install modes, and conversion support, etc. to the renderer (yet). In hindsight, I wish I had hidden everything inside the Convert() function for now to also reduce touch points with other parts of the code (leaving that for later).
I don't quite get the code suggesting. Seems its to rename PlainConverter to RegistryV1ToPlainBundleRenderer?
There was a problem hiding this comment.
Eventually I'd like to do away with the convert package all together in favor of the renderer.
I was talking about this - getting rid of the Converter in favor of using render.BundleRenderer directly, instead of wrapping renderer.BundleRender inside a Converter and then using Converter. That'd mean
package render type BundleRenderer struct{
.
.
}
func (r BundleRenderer) Convert (rv1 RegistryV1, installNamespace string, targetNamespaces []string) (*Plain, error)}
.
.
}
would have to be defined.
But that would also mean
carry over all the logic checking the install and target namespaces, and validating install modes, and conversion support, etc. to the renderer (yet)
has to be done. It didn't look like this'd be too much of a heavy lift to me, but maybe I'm missing something
There was a problem hiding this comment.
it's not about the effort, it's just about the scope of the change in the PR - and I'd also like to consider what to do with the Helm converter as part of this, and that will definitely influence everything up from that. I'd also like to think over whether that logic should live in the renderer itself or outside of it, or just in the Plain renderer, etc. It might have made things tidier, but I don't think it's necessary for this PR (though I do think its necessary).
perdasilva
commented
Apr 10, 2025
I started with that. But, because the convert function was so monolithic it meant each test would need to set itself up in a way that it can get through to the logic it wants to test, e.g. the csv would need to support AllNamespaces (which is not super important when you want to check that strategy deployment spec is properly converted to a deployment resource, etc. It also meant that, for the permissions tests, the resulting resource names would carry the hash (which is also not so important when you want to check that spec.permissions -> rbav1.Role and rbacv1.RoleBinding). Also, the convert function returns all generated resources, which means we need to try and find the expected ones in the pile. Not the end of the world, but adds complexity to write and in case of failure. So, I pivoted away towards adding the regression tests (#1895) , which, tbf might not be as thorough as unit testing, I thought it still gave me enough of an indication that everything would be ok after refactor. Having said all that, the generator unit tests were portable enough that I brought them over to the current rukpak/convert package (#1912). Maybe as we move through, their short livedness will outweigh their drawbacks and give us extra security. Wdyt? |
anik120
commented
Apr 10, 2025
That makes a lot of sense thank you for the context!! #1895 being present basically addresses the concern I was brining up, albeit in a different form - which makes sense given the challenges you described 👍🏽 |
744a674 to
8d924ddCompare8d924dd to
33dbb54Compare33dbb54 to
ea23274Compare| // WithSubjects applies rbac subjects to ClusterRoleBinding and RoleBinding resources | ||
| func WithSubjects(subjects ...rbacv1.Subject) func(client.Object) { | ||
| return func(obj client.Object) { | ||
| switch o := obj.(type) { |
There was a problem hiding this comment.
Add a default case to panic if the type doesn't support setting a subject?
There was a problem hiding this comment.
Same question for the other With* funcs.
joelanford
left a comment
There was a problem hiding this comment.
Looks great! One nit is all.
ea23274 to
30ad5f6Compare
Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com>
30ad5f6 to
f36e306Compare
Description
The
Convertfunction in the rukpak/convert package is very simplistic, hard to extend and lacking in testing. This PR adds a bundle rendering framework composed:ResourceGenerators added:
Reviewer Checklist