-
Notifications
You must be signed in to change notification settings - Fork 80
feat: Add Kubewarden resources to the basic and default resourceset #789
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
c9913bd
to
67ef478
Compare
Rebased, added missing Namespace backups. |
67ef478
to
3e7f9e3
Compare
Rebased, added policy-reporter subchart Secrets to the |
3e7f9e3
to
87a65e8
Compare
Rebased on top of main. Now it adds only to the |
Tested and automated by Kubewarden QA here => https://github.com/juadk/helm-charts/actions/runs/16470799097 ✅ |
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.
Overall this looks good. However, I'm worried about the possible performance implications of having so many rules that do not specify a namespace, implying in a query that searches through all resources of that kind in the entire cluster to find the ones which contain the desired label. Usually we avoid rules like that as much as possible and try to be namespace-specific.
I understand it might be the case that the resources you're trying to include could indeed be located in any namespace. If that is the case, I'd like to at least have these new rules be optional, something that only gets added to the ResourceSets if users explicitly opt-in. My suggestion would be adding a flag in the values file like Values.optionalResources.kubewarden.enabled
to control that.
If you need assistance in navigating the codebase to implement those changes let us know and we can help!
Thanks for the review! Good point. Applied suggestions, now:
|
@jbiers fixed the templating problem introduced with adding the optional Value. The resourcesets are under Renamed the new value to Added 2 new Hull tests for the change. |
b589707
to
0923e88
Compare
@viccuad I opened a PR to your PR (lol) to try to illustrate an approach closer to the behavior I meant by my previous comment: viccuad#1. I only considered the first two commits from your PR which is why my PR shows my branch as stale. Let me know if that is okay with you. Also pinging @mallardduck for a review on Monday as my PR changes how the operator builds its ResourceSets so having more people from the team on the know is good. |
@jbiers Yes, it's totally ok! Thanks for looking into this. Feel free to drop the redone commits from this PR by force pushing it, or to close it and open a new PR. Looking forward to have this merged. |
0923e88
to
87a65e8
Compare
The basic resourceset contains no Secrets. (https://ranchermanager.docs.rancher.com/reference-guides/backup-restore-configuration/backup-configuration#resourceset) Signed-off-by: Víctor Cuadrado Juan <[email protected]>
Signed-off-by: Víctor Cuadrado Juan <[email protected]>
1449e03
to
a9c6cae
Compare
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.
lgtm
Part of https://github.com/rancher/kubewarden/issues/7.
This PR adds Kubewarden resources to the basic resourceset (without Secrets) and the default resourceset (with Secrets).
This comprises:
catttle-kubewarden-system
(orcattle-kubewarden-*
), and the default Kubewarden Namespacekubewarden
.policy-reporter
subchart of thekubewarden-controller
chart, for theirdefault values. This doesn't include the Grafana integration nor other plugins.
The backup process doesn't include Secrets created to configure PolicyServers
for private registries unless those are correctly labeled by users.
Tested manually by,
applying also #787
and following the steps under kubewarden/docs#632
Documentation in Kubewarden side under:
kubewarden/docs#632