-
Notifications
You must be signed in to change notification settings - Fork 97
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
feat(namespaces): add empty namespace detection and removal #249
base: main
Are you sure you want to change the base?
Conversation
fixes #92 This PR is still WIP, as it lacks parallel processing of the namespaces as well as unit tests, could you please review if in principle it is what you'd be happy to accept ? |
Hi @isindir, Take a look at https://github.com/yonahd/kor/blob/main/CONTRIBUTING.md#repository-structure. |
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #249 +/- ##
==========================================
+ Coverage 43.58% 44.36% +0.78%
==========================================
Files 65 67 +2
Lines 4199 4355 +156
==========================================
+ Hits 1830 1932 +102
- Misses 2111 2157 +46
- Partials 258 266 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This is definitely a good direction(this is not a simple one). |
Helm chart I think needs extra changes:
I'm looking to work on unit tests if time allows these weekends. |
07bc31c
to
7246d82
Compare
Correct, currently the deployment is only used to export metrics, hence read-only RBAC, but we can consider going that way.
Interesting, especially when scanning unused resources in high-scaled clusters that might take more time than the interval set. We can continue with progress on both comments in separated issues/discussions (or in our Discord channel) as they are out-of-scope for this PR, but should be dippen into. I'd like to futher discuss them. Referring to https://github.com/yonahd/kor/blob/main/CONTRIBUTING.md#repository-structure was made to make sure you modify all required files, as adding any new unused resource support should address them. |
@doronkg ,
Thank you, I think I covered most of the files except helm chart and some unit tests, atm I'm trying to figure out if I can use fake clients to reliably test a feature I'm implementing. I deliberately left chart untouched as the change already looks very big. btw, for chart - it will be better to have fine-tuned RBAC - possibility to enable only those read/write/per namespace permissions per feature (resource). |
ac738c6
to
5b15ce6
Compare
@yonahd @luisdavim @doronkg, I finally found time and adding some more testing to the namespace removal - but I was not following if json configs with resources to skip from namespace evaluation for different k8s distros is fully implemented or not. Could you please suggest and review if my testing in |
The resource exceptions are merged fully. |
Let me know when this is ready for review. This is a massive pr and will take quite a while to review |
b6dd857
to
02a3f7e
Compare
@yonahd I feel that I'll not be able to do 100% unit test coverage for my new code, please review this PR. Thanks. |
d5d757e
to
1f091da
Compare
@yonahd - I rebased the PR to the latest, would you have time to review and may be merge it ? We could negotiate next course of actions if needed from my side on a call, please reach me out on keybase , same uid as here. |
Thanks for the PR |
Hi, sorry for the late response. |
4d854e2
to
fecedb6
Compare
… Identifier, as it contains Name and cannot be called Name
…pace & resourceInNamespace
…actor of fake dynamic client in tests
4960b73
to
199343d
Compare
What this PR does / why we need it?
This PR finds empty and non-default kubernetes namespaces, and if instructed removes these from the cluster.
PR Checklist
GitHub Issue
Closes [#92]
Notes for your reviewers