Feat/crd argo rollouts #375

oliveiraxavier · 2024-10-30T01:32:13Z

This PR allow get some unused Argo rollouts

PR Checklist

This PR adds K8s exceptions (false positives)
This PR adds new code
This PR includes tests for new/existing code
This PR adds docs

Notes for your reviewers

This PR adds support for get unused Argo Rollout, Cluster Analysis templates and Analysis templates.

For this pr, some code refactoring was necessary.

codecov-commenter · 2024-10-30T06:36:34Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 42.85714% with 212 lines in your changes missing coverage. Please review.

Project coverage is 44.25%. Comparing base (9cd0aba) to head (e85e81e).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/clusterconfig/connection.go	11.68%	64 Missing and 4 partials ⚠️
pkg/kor/argorollouts.go	56.16%	48 Missing and 16 partials ⚠️
pkg/kor/all.go	46.87%	16 Missing and 1 partial ⚠️
pkg/filters/options.go	0.00%	12 Missing ⚠️
pkg/kor/exporter.go	0.00%	7 Missing ⚠️
cmd/kor/root.go	0.00%	6 Missing ⚠️
cmd/kor/all.go	0.00%	5 Missing ⚠️
cmd/kor/exporter.go	0.00%	5 Missing ⚠️
pkg/kor/formatter.go	33.33%	3 Missing and 1 partial ⚠️
cmd/kor/crds.go	0.00%	2 Missing ⚠️
... and 21 more

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #375      +/-   ##
==========================================
+ Coverage   43.38%   44.25%   +0.87%     
==========================================
  Files          63       65       +2     
  Lines        4020     4291     +271     
==========================================
+ Hits         1744     1899     +155     
- Misses       2030     2128      +98     
- Partials      246      264      +18

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

yonahd · 2024-10-30T21:39:33Z

@oliveiraxavier Please rebase so we can properly review the code

oliveiraxavier · 2024-10-31T01:32:58Z

@yonahd The main branch has been merged with the feat branch.

yonahd · 2024-10-31T05:01:58Z

@yonahd The main branch has been merged with the feat branch.

Did you sync your fork?

oliveiraxavier · 2024-10-31T23:46:59Z

@yonahd The main branch has been merged with the feat branch.

Did you sync your fork?

Yes, synced

yonahd · 2024-11-05T18:28:25Z

@oliveiraxavier I'm working on the review. Any chance you can split this into 2 separate PRs
One for the Argo rollout and a second for moving the create client logic into a separate folder?

oliveiraxavier · 2024-11-05T21:50:40Z

@oliveiraxavier I'm working on the review. Any chance you can split this into 2 separate PRs One for the Argo rollout and a second for moving the create client logic into a separate folder?

In my previous PR (#363), the logic was separated but the resource search files (deployments for example) were extremely tied to the connection logic, I separated them with that in mind. Refactoring this would almost be a reopening of the previous pr and would require some more refactoring of everything I developed, I can't see how I can split this PR into two without breaking the functioning of everything.

yonahd · 2024-11-06T06:14:14Z

@oliveiraxavier I'm working on the review. Any chance you can split this into 2 separate PRs One for the Argo rollout and a second for moving the create client logic into a separate folder?

In my previous PR (#363), the logic was separated but the resource search files (deployments for example) were extremely tied to the connection logic, I separated them with that in mind. Refactoring this would almost be a reopening of the previous pr and would require some more refactoring of everything I developed, I can't see how I can split this PR into two without breaking the functioning of everything.

Maybe I'm missing something but it seems most of the connection logic is just moving the logic from kor.go
If it's too much to separate I'll review it as it is (mainly wanted the separation to help with my adhd)

oliveiraxavier · 2024-11-06T06:33:46Z

Talvez eu esteja esquecendo de algo, mas parece que a maior parte da lógica de conexão está apenas movendo a lógica do kor.go. Se for muita coisa para separar, revisarei como está (queria principalmente a separação para ajudar com meu TDAH)

I won't be able to work on it at the moment

yonahd · 2024-11-06T06:38:38Z

Talvez eu esteja esquecendo de algo, mas parece que a maior parte da lógica de conexão está apenas movendo a lógica do kor.go. Se for muita coisa para separar, revisarei como está (queria principalmente a separação para ajudar com meu TDAH)

I won't be able to work on it at the moment

No worries. I'll review it as is.
It'll just take me a bit longer

yonahd

Logic looks right for the finding unused argorollouts
There are some structure issues and minor fixes.
I suggest you also look through https://github.com/yonahd/kor/blob/main/CONTRIBUTING.md which will help structuring the PR.
Also we need to think of how we give the permissions in the helm chart

yonahd · 2024-11-05T18:15:16Z

README.md

@@ -253,6 +269,45 @@ Unused resources in namespace: "test"
 +---+----------------+----------------------------------------------+--------------------------------------------------------+
 ```

+```sh
+kor all --include-third-party-crds argo-rollouts,argo-rollouts-analysis-templates,argo-rollouts-cluster-analysis-templates --show-reason --show-reason


No need for this additional example

yonahd · 2024-11-05T18:18:38Z

cmd/kor/root.go

@@ -85,4 +87,5 @@ func addFilterOptionsFlag(cmd *cobra.Command, opts *filters.Options) {
 	cmd.PersistentFlags().StringVar(&opts.IncludeLabels, "include-labels", opts.IncludeLabels, "Selector to filter in, Example: --include-labels key1=value1.(currently supports one label)")
 	cmd.PersistentFlags().StringSliceVarP(&opts.ExcludeNamespaces, "exclude-namespaces", "e", opts.ExcludeNamespaces, "Namespaces to be excluded, split by commas. Example: --exclude-namespaces ns1,ns2,ns3. If --include-namespaces is set, --exclude-namespaces will be ignored.")
 	cmd.PersistentFlags().StringSliceVarP(&opts.IncludeNamespaces, "include-namespaces", "n", opts.IncludeNamespaces, "Namespaces to run on, split by commas. Example: --include-namespaces ns1,ns2,ns3. If set, non-namespaced resources will be ignored.")
+	rootCmd.PersistentFlags().StringSliceVar(&opts.IncludeThirdPartyCrds, "include-third-party-crds", opts.IncludeThirdPartyCrds, "Custom resources defintions to search, split by commas. Example: --include-crds argo-rollouts. If set, the chosen crd will be returned (in addition to the other resources, of course)")


We can allow the user to select specific ones without a flag using the multi functionality.
I think the flag should exist in case the user chooses kor all and wants to choose either only native resources or also crds (a boolean flag)

yonahd · 2024-11-05T18:22:31Z

pkg/kor/argorollouts.go

Please change the folder structure to pkg/kor/crds

This should cause circular import error

yonahd · 2024-11-05T18:24:15Z

pkg/kor/create_test_resources.go

Create a separate file create_test_crd_resources.go for crd test resources

yonahd · 2024-11-06T19:16:53Z

pkg/clusterconfig/connection.go

Please move this into utils

yonahd · 2024-11-06T19:20:15Z

pkg/kor/formatter.go

@@ -235,3 +235,12 @@ func FormatOutputAll(namespace string, allDiffs []ResourceDiff, opts common.Opts
 	table.Render()
 	return fmt.Sprintf("Unused resources in namespace: %q\n%s\n", namespace, buf.String())
 }
+
+func SkipIfContainsValue(data []ResourceInfo, key string, value interface{}) bool {


This Should not be in formatter.go

yonahd · 2024-11-06T19:31:51Z

pkg/kor/argorollouts.go

+	"github.com/yonahd/kor/pkg/filters"
+)
+
+func GetUnusedArgoRollouts(clientsetinterface clusterconfig.ClientInterface, namespace string, filterOpts *filters.Options) ResourceDiff {


We need to create cmd/kor/argorollouts.go so users can select this one specifically
Also missing pkg/kor/multi.go - allow finding your new resource in a comma-separated query along other resources.

yonahd · 2024-11-06T19:38:45Z

pkg/kor/argorollouts.go

+	"github.com/yonahd/kor/pkg/clusterconfig"
+	"github.com/yonahd/kor/pkg/filters"
+)
+


nit: move the get unused functions to the bottom to match convention

yonahd · 2024-11-06T19:44:16Z

pkg/kor/config_test.go

What is this needed for?

oliveiraxavier · 2024-11-10T15:36:09Z

I'm closing this pr.
My time is short and until I finish these changes there will be many conflicts.

oliveiraxavier · 2024-11-10T15:36:32Z

Thank's for your review

yonahd · 2024-11-10T17:00:39Z

I'm closing this pr. My time is short and until I finish these changes there will be many conflicts.

Are you planning on completing this?

oliveiraxavier · 2024-11-10T17:36:34Z

I'm closing this pr. My time is short and until I finish these changes there will be many conflicts.

Are you planning on completing this?

Maybe in two or three months I can set aside some time to get back to it. If someone follows the idea, no problem.

oliveiraxavier added 2 commits November 2, 2024 21:16

Rebase, lint, test and sync

4fa6651

Rebase, lint, test and sync charts

e85e81e

oliveiraxavier force-pushed the feat/crd-argo-rollouts branch 2 times, most recently from 1e937ad to e85e81e Compare November 3, 2024 00:31

yonahd reviewed Nov 6, 2024

View reviewed changes

oliveiraxavier closed this Nov 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/crd argo rollouts #375

Feat/crd argo rollouts #375

oliveiraxavier commented Oct 30, 2024 •

edited

Loading

codecov-commenter commented Oct 30, 2024 •

edited

Loading

yonahd commented Oct 30, 2024

oliveiraxavier commented Oct 31, 2024

yonahd commented Oct 31, 2024

oliveiraxavier commented Oct 31, 2024

yonahd commented Nov 5, 2024

oliveiraxavier commented Nov 5, 2024

yonahd commented Nov 6, 2024

oliveiraxavier commented Nov 6, 2024

yonahd commented Nov 6, 2024

yonahd left a comment

yonahd Nov 5, 2024

yonahd Nov 5, 2024

yonahd Nov 5, 2024

oliveiraxavier Nov 10, 2024

yonahd Nov 5, 2024

yonahd Nov 6, 2024

yonahd Nov 6, 2024

yonahd Nov 6, 2024

yonahd Nov 6, 2024

yonahd Nov 6, 2024

oliveiraxavier commented Nov 10, 2024

oliveiraxavier commented Nov 10, 2024

yonahd commented Nov 10, 2024

oliveiraxavier commented Nov 10, 2024

Feat/crd argo rollouts #375

Feat/crd argo rollouts #375

Conversation

oliveiraxavier commented Oct 30, 2024 • edited Loading

PR Checklist

Notes for your reviewers

codecov-commenter commented Oct 30, 2024 • edited Loading

Codecov Report

yonahd commented Oct 30, 2024

oliveiraxavier commented Oct 31, 2024

yonahd commented Oct 31, 2024

oliveiraxavier commented Oct 31, 2024

yonahd commented Nov 5, 2024

oliveiraxavier commented Nov 5, 2024

yonahd commented Nov 6, 2024

oliveiraxavier commented Nov 6, 2024

yonahd commented Nov 6, 2024

yonahd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliveiraxavier commented Nov 10, 2024

oliveiraxavier commented Nov 10, 2024

yonahd commented Nov 10, 2024

oliveiraxavier commented Nov 10, 2024

oliveiraxavier commented Oct 30, 2024 •

edited

Loading

codecov-commenter commented Oct 30, 2024 •

edited

Loading