cleanup: Fix various issues found by code inspection #1278

akrejcir · 2025-02-17T13:33:54Z

What this PR does / why we need it:
Fixed:

Handle unhandled errors
Removed unused code
Stopped using deprecated methods
Code style issues

Release note:

None

akrejcir · 2025-02-17T13:36:35Z

pkg/monitoring/metrics/template-validator/metrics.go

-func ListMetrics() []operatormetrics.Metric {
-	return operatormetrics.ListMetrics()
-}


@machadovilaca , I've found this unused function and removed it. Is this a bug, that it is not used anywhere?

In usually call these functions the tooling to generate the docs, but when setting up multiple metrics, in the registry we only need to list from one. That's why the ListMetrics from metrics/ssp-operator is used but not this one

So, this was not a bug, that the function was not called?

No, we can remove it. On a future release of the operator-observability library, it will support different registries, and then we will improve the docs to setup and list the metrics separately.

akrejcir · 2025-02-17T13:36:59Z

/cc @0xFelix @jcanocan

0xFelix · 2025-02-17T13:40:51Z

internal/env/env_test.go

 		res := GetOperatorVersion()
 		Expect(res).To(Equal("v0.0.1"), "OPERATOR_VERSION should equal")
-		os.Unsetenv(OperatorVersionKey)


Maybe DeferCleanup that so the Env is always unset?

0xFelix · 2025-02-17T13:42:15Z

internal/template-validator/validator/app.go

@@ -130,7 +130,10 @@ func (app *App) Run() {

 func registerReadinessProbe() {
 	http.HandleFunc("/readyz", func(resp http.ResponseWriter, req *http.Request) {
-		fmt.Fprintf(resp, "ok")
+		_, err := fmt.Fprintf(resp, "ok")
+		if err != nil {


0xFelix · 2025-02-17T13:44:25Z

internal/template-validator/validator/app.go

@@ -130,7 +130,10 @@ func (app *App) Run() {

 func registerReadinessProbe() {
 	http.HandleFunc("/readyz", func(resp http.ResponseWriter, req *http.Request) {
-		fmt.Fprintf(resp, "ok")
+		_, err := fmt.Fprintf(resp, "ok")


resp.Write([]byte("ok")) would probably achieve the same without an additional dependency.

Good idea, done.

0xFelix · 2025-02-17T13:45:24Z

tests/crypto_policy_test.go

@@ -204,7 +204,7 @@ func getCaCertificate() []byte {
 func tryToAccessEndpoint(pod core.Pod, serviceName string, subpath string, port uint16, tlsConfig clientTLSOptions, insecure bool) (attemptedUrl string, err error) {
 	conn, err := portForwarder.Connect(&pod, port)
 	Expect(err).ToNot(HaveOccurred())
-	defer conn.Close()
+	defer Expect(conn.Close()).To(Succeed())


Will it run if another Expect fails?

Yes, it will work. A panic can happen during panicking. They will just wrap around each other.
https://go.dev/play/p/Eq5sdpNkDl

0xFelix · 2025-02-17T13:47:14Z

tests/port-forwarding.go

@@ -43,17 +44,19 @@ func (p *portForwarderImpl) Connect(pod *core.Pod, remotePort uint16) (net.Conn,
 	headers.Set(core.PortForwardRequestIDHeader, strconv.Itoa(int(requestId)))
 	errorStream, err := streamConnection.CreateStream(headers)
 	if err != nil {
-		streamConnection.Close()
-		return nil, err
+		closeErr := streamConnection.Close()


0xFelix · 2025-02-17T13:47:19Z

tests/port-forwarding.go


 	headers.Set(core.StreamType, core.StreamTypeData)
 	dataStream, err := streamConnection.CreateStream(headers)
 	if err != nil {
-		streamConnection.Close()
-		return nil, err
+		closeErr := streamConnection.Close()


0xFelix · 2025-02-17T13:47:40Z

tests/validator_test.go

@@ -3,6 +3,7 @@ package tests
 import (
 	"crypto/tls"
 	"crypto/x509"
+	goerrors "errors"


Import "k8s.io/apimachinery/pkg/api/errors" as k8serrors instead?

akrejcir · 2025-02-18T08:40:17Z

In multiple PRs we have problem with the golangci-lint timeout. I will look into it...

akrejcir · 2025-02-18T08:40:47Z

/retest

akrejcir · 2025-02-18T11:12:37Z

/retest

0xFelix

/approve

Thanks!

kubevirt-bot · 2025-02-18T12:31:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 0xFelix

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [0xFelix]

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

They were found by Goland code inspection. Signed-off-by: Andrej Krejcir <[email protected]>

Signed-off-by: Andrej Krejcir <[email protected]>

codingben

/lgtm
/hold

@akrejcir Do you want to hold it until you get a response from @machadovilaca to your question about an unused function?

The unhandled errors were found by Goland code inspection. Signed-off-by: Andrej Krejcir <[email protected]>

sonarqubecloud · 2025-02-19T09:28:02Z

Quality Gate failed

Failed conditions
3.8% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

akrejcir · 2025-02-19T10:58:27Z

/unhold

akrejcir · 2025-02-20T08:17:11Z

/retest

jcanocan · 2025-02-20T08:45:44Z

/lgtm

openshift-ci · 2025-02-20T10:07:42Z

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

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-upgrade-functests	`c7b19d4`	link	true	`/test e2e-upgrade-functests`
ci/prow/e2e-functests	`c7b19d4`	link	true	`/test e2e-functests`
ci/prow/e2e-single-node-functests	`c7b19d4`	link	true	`/test e2e-single-node-functests`

Full PR test history. Your PR dashboard.

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 release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Feb 17, 2025

kubevirt-bot requested review from 0xFelix and codingben February 17, 2025 13:33

kubevirt-bot added the size/L label Feb 17, 2025

akrejcir commented Feb 17, 2025

View reviewed changes

kubevirt-bot requested a review from jcanocan February 17, 2025 13:37

0xFelix reviewed Feb 17, 2025

View reviewed changes

akrejcir force-pushed the code-inspection-cleanup branch 2 times, most recently from c3d1c4b to 45c1ccc Compare February 17, 2025 14:52

0xFelix reviewed Feb 18, 2025

View reviewed changes

kubevirt-bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 18, 2025

akrejcir added 3 commits February 19, 2025 10:02

cleanup: Fix code style issues

6dbee28

They were found by Goland code inspection. Signed-off-by: Andrej Krejcir <[email protected]>

cleanup: Remove unused code and redundant types

4b814c4

Signed-off-by: Andrej Krejcir <[email protected]>

cleanup: Don't use deprecated methods and fields

7f8befc

Signed-off-by: Andrej Krejcir <[email protected]>

codingben approved these changes Feb 19, 2025

View reviewed changes

kubevirt-bot assigned codingben Feb 19, 2025

kubevirt-bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Feb 19, 2025

cleanup: Handle errors

c7b19d4

The unhandled errors were found by Goland code inspection. Signed-off-by: Andrej Krejcir <[email protected]>

akrejcir force-pushed the code-inspection-cleanup branch from 45c1ccc to c7b19d4 Compare February 19, 2025 09:27

kubevirt-bot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 19, 2025

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 19, 2025

kubevirt-bot assigned jcanocan Feb 20, 2025

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Feb 20, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cleanup: Fix various issues found by code inspection #1278

cleanup: Fix various issues found by code inspection #1278

akrejcir commented Feb 17, 2025

akrejcir Feb 17, 2025

machadovilaca Feb 19, 2025

akrejcir Feb 19, 2025

machadovilaca Feb 19, 2025

akrejcir commented Feb 17, 2025

0xFelix Feb 17, 2025

akrejcir Feb 17, 2025

0xFelix Feb 17, 2025

akrejcir Feb 17, 2025

0xFelix Feb 17, 2025

akrejcir Feb 17, 2025

0xFelix Feb 17, 2025

akrejcir Feb 17, 2025

0xFelix Feb 17, 2025

akrejcir Feb 17, 2025

0xFelix Feb 17, 2025

akrejcir Feb 17, 2025

0xFelix Feb 17, 2025

akrejcir Feb 17, 2025

akrejcir commented Feb 18, 2025

akrejcir commented Feb 18, 2025

akrejcir commented Feb 18, 2025

0xFelix left a comment

kubevirt-bot commented Feb 18, 2025

codingben left a comment

sonarqubecloud bot commented Feb 19, 2025

akrejcir commented Feb 19, 2025

akrejcir commented Feb 20, 2025

jcanocan commented Feb 20, 2025

openshift-ci bot commented Feb 20, 2025

cleanup: Fix various issues found by code inspection #1278

Are you sure you want to change the base?

cleanup: Fix various issues found by code inspection #1278

Conversation

akrejcir commented Feb 17, 2025

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

akrejcir commented Feb 17, 2025

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akrejcir commented Feb 18, 2025

akrejcir commented Feb 18, 2025

akrejcir commented Feb 18, 2025

0xFelix left a comment

Choose a reason for hiding this comment

kubevirt-bot commented Feb 18, 2025

codingben left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Feb 19, 2025

Quality Gate failed

akrejcir commented Feb 19, 2025

akrejcir commented Feb 20, 2025

jcanocan commented Feb 20, 2025

openshift-ci bot commented Feb 20, 2025