pkg/components: Refactor to reduce duplicate code #1845

RamLavi · 2024-08-06T11:45:03Z

What this PR does / why we need it:
currently sonarCloud reports a lot of code is duplicate on pkg/components (37.6%).
Refactoring the code to reduce duplicate to 0% and make thing more clear (all policies are visible in one screen, no endless scrolling).

Special notes for your reviewer:

Release note:

NONE

kubevirt-bot · 2024-08-06T11:45:06Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from ramlavi. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

RamLavi · 2024-08-06T11:51:57Z

@oshoval @phoracek PTAL

oshoval · 2024-08-06T11:57:57Z

pkg/components/components.go

+	var rules []rbacv1.PolicyRule
+
+	rules = append(rules,
+		newPolicyRule([]string{""}, []string{"events"}, []string{"update"}),


wdyt about this ?
easier to read imo please

newPolicyRule( []string{""}, []string{"events"}, []string{"update"} ),

ouch sonar is not happy from that
i guess we dont have a choice

tried: duplicity went up 18%
perhaps we could use new line only on lines that are too long?

either that, or just revert to the initial please (better imo)
whatever you prefer
thanks

oshoval · 2024-08-06T12:13:26Z

beside cosmetics there are no changes right ?

RamLavi · 2024-08-06T12:27:10Z

beside cosmetics there are no changes right ?

It's only a refactor. No logic changed.

nunnatsa · 2024-08-11T09:02:52Z

pkg/components/components.go

+	var rules []rbacv1.PolicyRule
+
+	rules = append(rules,
+		newPolicyRule([]string{"apps"}, []string{"daemonsets"}, []string{"get", "create", "update", "delete"}),
+		newPolicyRule([]string{""}, []string{"configmaps"}, []string{"get", "create", "update"}),
+		newPolicyRule([]string{"apps"}, []string{"deployments"}, []string{"delete"}),
+		newPolicyRule([]string{""}, []string{"namespaces"}, []string{"update", "get", "patch"}),
+		newPolicyRule([]string{""}, []string{"serviceaccounts"}, []string{"get", "create", "update", "delete"}),
+		newPolicyRule([]string{"monitoring.coreos.com"}, []string{"prometheusrules", "servicemonitors"}, []string{"get", "create", "update", "delete"}),
+		newPolicyRule([]string{"rbac.authorization.k8s.io"}, []string{"roles", "rolebindings"}, []string{"get", "create", "update", "delete"}),
+		newPolicyRule([]string{"policy"}, []string{"poddisruptionbudgets"}, []string{"get", "delete"}),
+		newPolicyRule([]string{""}, []string{"configmaps"}, []string{"patch"}),
+		newPolicyRule([]string{"coordination.k8s.io"}, []string{"leases"}, []string{"get", "list", "watch", "create", "update", "patch", "delete"}),
+		newPolicyRule([]string{"cert-manager.io"}, []string{"certificates", "issuers"}, []string{"get", "create", "update", "delete"}),
+	)


why using append? append is pricey. I think you can initialize the slice with the required data; e.g.

Suggested change

var rules []rbacv1.PolicyRule

rules = append(rules,

newPolicyRule([]string{"apps"}, []string{"daemonsets"}, []string{"get", "create", "update", "delete"}),

newPolicyRule([]string{""}, []string{"configmaps"}, []string{"get", "create", "update"}),

newPolicyRule([]string{"apps"}, []string{"deployments"}, []string{"delete"}),

newPolicyRule([]string{""}, []string{"namespaces"}, []string{"update", "get", "patch"}),

newPolicyRule([]string{""}, []string{"serviceaccounts"}, []string{"get", "create", "update", "delete"}),

newPolicyRule([]string{"monitoring.coreos.com"}, []string{"prometheusrules", "servicemonitors"}, []string{"get", "create", "update", "delete"}),

newPolicyRule([]string{"rbac.authorization.k8s.io"}, []string{"roles", "rolebindings"}, []string{"get", "create", "update", "delete"}),

newPolicyRule([]string{"policy"}, []string{"poddisruptionbudgets"}, []string{"get", "delete"}),

newPolicyRule([]string{""}, []string{"configmaps"}, []string{"patch"}),

newPolicyRule([]string{"coordination.k8s.io"}, []string{"leases"}, []string{"get", "list", "watch", "create", "update", "patch", "delete"}),

newPolicyRule([]string{"cert-manager.io"}, []string{"certificates", "issuers"}, []string{"get", "create", "update", "delete"}),

)

rules := []rbacv1.PolicyRule{

newPolicyRule([]string{"apps"}, []string{"daemonsets"}, []string{"get", "create", "update", "delete"}),

newPolicyRule([]string{""}, []string{"configmaps"}, []string{"get", "create", "update"}),

newPolicyRule([]string{"apps"}, []string{"deployments"}, []string{"delete"}),

newPolicyRule([]string{""}, []string{"namespaces"}, []string{"update", "get", "patch"}),

newPolicyRule([]string{""}, []string{"serviceaccounts"}, []string{"get", "create", "update", "delete"}),

newPolicyRule([]string{"monitoring.coreos.com"}, []string{"prometheusrules", "servicemonitors"}, []string{"get", "create", "update", "delete"}),

newPolicyRule([]string{"rbac.authorization.k8s.io"}, []string{"roles", "rolebindings"}, []string{"get", "create", "update", "delete"}),

newPolicyRule([]string{"policy"}, []string{"poddisruptionbudgets"}, []string{"get", "delete"}),

newPolicyRule([]string{""}, []string{"configmaps"}, []string{"patch"}),

newPolicyRule([]string{"coordination.k8s.io"}, []string{"leases"}, []string{"get", "list", "watch", "create", "update", "patch", "delete"}),

newPolicyRule([]string{"cert-manager.io"}, []string{"certificates", "issuers"}, []string{"get", "create", "update", "delete"}),

}

nunnatsa · 2024-08-11T09:03:20Z

pkg/components/components.go

 	}
 	return role
 }

 func GetClusterRole(allowMultus bool) *rbacv1.ClusterRole {
+	var rules []rbacv1.PolicyRule


nunnatsa · 2024-08-11T09:03:52Z

pkg/components/components.go

+	var rules []rbacv1.PolicyRule
+
+	rules = append(rules,


RamLavi · 2024-08-13T09:36:18Z

Change: Rebase + removed @oshoval 's suggestion as it doesn't fix duplicate code issue

RamLavi · 2024-08-13T09:36:55Z

Change: Fix for @nunnatsa review

RamLavi · 2024-08-13T09:38:13Z

/hold
I want to manually check that no rbac was left out

oshoval · 2024-08-13T09:39:46Z

/hold I want to manually check that no rbac was left out

if any are left out, tests should find it, or we are missing tests, or those arent needed anymore
so if you find any, worth to check which of those reasons is the right one please

oshoval · 2024-08-13T09:56:03Z

you didnt rebase correctly, please check it
those seem to be missed, it will fail on OpenShift, but not on U/S (well it will also fail here as part of the deployment)
"virtualmachines/finalizers",
"virtualmachineinstances/finalizers",

Those need to be included please:
25b584e

RamLavi · 2024-08-13T10:40:08Z

virtualmachines/finalizers

thanks!

sonarqubecloud · 2024-08-13T10:41:11Z

Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

oshoval · 2024-08-13T10:41:57Z

https://github.com/kubevirt/cluster-network-addons-operator/compare/968a49b0b0b0cee146b1409e44d9bebd457fd49e..cc6fe21598b5e551d09a187b03c9247b0dc4b3d3
you have here twice update

kubevirt-bot · 2024-11-11T14:44:42Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

/lifecycle stale

kubevirt-bot · 2024-12-11T15:29:42Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

/lifecycle rotten

This is done in order to eliminate sonarCloud issue. Signed-off-by: Ram Lavi <[email protected]>

sonarqubecloud · 2024-12-15T07:48:02Z

Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

RamLavi · 2024-12-15T07:49:06Z

Change: Rebase

RamLavi · 2024-12-15T07:49:19Z

https://github.com/kubevirt/cluster-network-addons-operator/compare/968a49b0b0b0cee146b1409e44d9bebd457fd49e..cc6fe21598b5e551d09a187b03c9247b0dc4b3d3 you have here twice update

nice catch! - fixed. PTAL

RamLavi · 2024-12-15T07:50:37Z

/hold cancel

RamLavi · 2024-12-15T07:52:24Z

/remove-lifecycle rotten

kubevirt-bot · 2024-12-15T07:52:42Z

@RamLavi: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-e2e-cnao-multus-dynamic-networks-functests	`519dec3`	link	true	`/test pull-e2e-cnao-multus-dynamic-networks-functests`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

kubevirt-bot added the release-note-none Denotes a PR that doesn't merit a release note. label Aug 6, 2024

kubevirt-bot requested a review from phoracek August 6, 2024 11:45

kubevirt-bot added the dco-signoff: yes Indicates the PR's author has DCO signed all their commits. label Aug 6, 2024

kubevirt-bot requested a review from qinqon August 6, 2024 11:45

kubevirt-bot added the size/XL label Aug 6, 2024

oshoval reviewed Aug 6, 2024

View reviewed changes

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 8, 2024

nunnatsa reviewed Aug 11, 2024

View reviewed changes

pkg/components/components.go Outdated

Comment on lines 479 to 439

var rules []rbacv1.PolicyRule

rules = append(rules,

Copy link

Contributor

nunnatsa Aug 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

RamLavi force-pushed the refactor_components branch from 5d31b26 to 6896c42 Compare August 13, 2024 09:35

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 13, 2024

RamLavi force-pushed the refactor_components branch from 6896c42 to 968a49b Compare August 13, 2024 09:36

kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 13, 2024

RamLavi force-pushed the refactor_components branch from 968a49b to cc6fe21 Compare August 13, 2024 10:40

kubevirt-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 11, 2024

kubevirt-bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Dec 11, 2024

pkg/components: Refactor to reduce duplicate code

519dec3

This is done in order to eliminate sonarCloud issue. Signed-off-by: Ram Lavi <[email protected]>

RamLavi force-pushed the refactor_components branch from cc6fe21 to 519dec3 Compare December 15, 2024 07:47

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 15, 2024

kubevirt-bot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Dec 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/components: Refactor to reduce duplicate code #1845

pkg/components: Refactor to reduce duplicate code #1845

RamLavi commented Aug 6, 2024 •

edited

Loading

kubevirt-bot commented Aug 6, 2024

RamLavi commented Aug 6, 2024

oshoval Aug 6, 2024 •

edited

Loading

oshoval Aug 6, 2024 •

edited

Loading

RamLavi Aug 6, 2024

oshoval Aug 6, 2024 •

edited

Loading

oshoval commented Aug 6, 2024

RamLavi commented Aug 6, 2024

nunnatsa Aug 11, 2024

nunnatsa Aug 11, 2024

nunnatsa Aug 11, 2024

RamLavi commented Aug 13, 2024

RamLavi commented Aug 13, 2024 •

edited

Loading

RamLavi commented Aug 13, 2024

oshoval commented Aug 13, 2024

oshoval commented Aug 13, 2024 •

edited

Loading

RamLavi commented Aug 13, 2024

sonarqubecloud bot commented Aug 13, 2024

oshoval commented Aug 13, 2024

kubevirt-bot commented Nov 11, 2024

kubevirt-bot commented Dec 11, 2024

sonarqubecloud bot commented Dec 15, 2024

RamLavi commented Dec 15, 2024

RamLavi commented Dec 15, 2024

RamLavi commented Dec 15, 2024

RamLavi commented Dec 15, 2024

kubevirt-bot commented Dec 15, 2024 •

edited

Loading

pkg/components: Refactor to reduce duplicate code #1845

Are you sure you want to change the base?

pkg/components: Refactor to reduce duplicate code #1845

Conversation

RamLavi commented Aug 6, 2024 • edited Loading

kubevirt-bot commented Aug 6, 2024

RamLavi commented Aug 6, 2024

oshoval Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

oshoval Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

RamLavi Aug 6, 2024

Choose a reason for hiding this comment

oshoval Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

oshoval commented Aug 6, 2024

RamLavi commented Aug 6, 2024

nunnatsa Aug 11, 2024

Choose a reason for hiding this comment

nunnatsa Aug 11, 2024

Choose a reason for hiding this comment

nunnatsa Aug 11, 2024

Choose a reason for hiding this comment

RamLavi commented Aug 13, 2024

RamLavi commented Aug 13, 2024 • edited Loading

RamLavi commented Aug 13, 2024

oshoval commented Aug 13, 2024

oshoval commented Aug 13, 2024 • edited Loading

RamLavi commented Aug 13, 2024

sonarqubecloud bot commented Aug 13, 2024

Quality Gate passed

oshoval commented Aug 13, 2024

kubevirt-bot commented Nov 11, 2024

kubevirt-bot commented Dec 11, 2024

sonarqubecloud bot commented Dec 15, 2024

Quality Gate passed

RamLavi commented Dec 15, 2024

RamLavi commented Dec 15, 2024

RamLavi commented Dec 15, 2024

RamLavi commented Dec 15, 2024

kubevirt-bot commented Dec 15, 2024 • edited Loading

RamLavi commented Aug 6, 2024 •

edited

Loading

oshoval Aug 6, 2024 •

edited

Loading

oshoval Aug 6, 2024 •

edited

Loading

oshoval Aug 6, 2024 •

edited

Loading

RamLavi commented Aug 13, 2024 •

edited

Loading

oshoval commented Aug 13, 2024 •

edited

Loading

kubevirt-bot commented Dec 15, 2024 •

edited

Loading