community: cleanup code, reduce complexity, add make targets #368

dhiller · 2024-12-11T17:40:30Z

What this PR does / why we need it:

Reduces complexity in:

validators/cmd/sigs-validator
generators/cmd/devstats

Adds make targets:

lint - will run golangci-lint, additionally gocyclo with > 10
test - will run go tests

/kind cleanup
/wg code-quality

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

kubevirt-bot · 2024-12-11T17:40:33Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

kubevirt-bot · 2024-12-11T17:40:35Z

[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 assign rmohr for approval. 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

Based on PR kubevirt/community#368 where new make targets test and lint are introduced we are adding new presubmit jobs that execute them. Signed-off-by: Daniel Hiller <[email protected]>

Based on PR kubevirt/community#368 where new make targets test, lint and coverage are introduced we are adding new presubmit jobs that execute them. Signed-off-by: Daniel Hiller <[email protected]>

aburdenthehand · 2024-12-16T09:20:38Z

/cc @EdDev for wg-code-quality

kubevirt-bot · 2024-12-16T09:20:43Z

@aburdenthehand: GitHub didn't allow me to request PR reviews from the following users: for.

Note that only kubevirt members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @EdDev for wg-code-quality

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.

xpivarc · 2025-01-07T08:58:22Z

validators/cmd/sigs/sigs-validator_test.go

+	"testing"
+)
+
+func Test_validateGroups(t *testing.T) {


Is the _ convention? I would expect to see TestValidateGroups

Hm, good q - I believe that was an autogenerated test func name that I didn't really look at.

@xpivarc I investigated a bit - reason for this weird name is that Goland has auto-generated the func name for the non-exported func that is tested.

Now if I rename the func as you requested the IDE will not find the tested func anymore.

Now my q is: does it matter that much? I'd rather expect the test func name to be as close to the name of the func it tests. WDYT?

Well that is weird, see https://go.dev/doc/code#Testing where TestXXX should work. I won't insist but my eye found it weird.

I agree that for exported functions it should be as you said, however for non-exported functions whose name is starting with a lowercase letter the usage of an underscore might be more obvious.

Compare

func DoFuncExported() {} func TestDoFuncExported() {}

to

func doFuncNonExported() {} func Test_doFuncNonExported() {}

where the latter is the matching case here.

And again - this is just to help the IDE to find the test for the func and the func for the test func.

lyarwood · 2025-01-09T10:30:33Z

/cc

lyarwood · 2025-01-15T10:35:58Z

Makefile

+	go test ./... -coverprofile=/tmp/coverage.out
+	covreport  -i /tmp/coverage.out -o ${COVERAGE_OUTPUT_PATH}
+
+install-metrics-binaries:


nit - Any reason to have another target with an unrelated name for installing a linting tool?

probably over-engineering :)

lyarwood · 2025-01-15T10:41:15Z

Makefile

+	covreport  -i /tmp/coverage.out -o ${COVERAGE_OUTPUT_PATH}
+
+install-metrics-binaries:
+	if ! command -V golangci-lint; then curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b ${GOPATH}/bin ${GOLANGCI_LINT_VERSION} ; fi


nit - It would be nicer to install this into a local bin to avoid changing the users version

https://github.com/kubevirt/common-instancetypes/blob/cef52ded0abde7b37564faa619aa745d10b5e335/Makefile#L151-L154

lyarwood · 2025-01-15T10:41:49Z

.golangci.yaml

+  - gocyclo
+linters-settings:
+  gocyclo:
+    min-complexity: 10


Missing new line

Adds a make target `lint` that will be used to run enabled linters, which are configured via .golangci.yaml . Initially only the default linters are enabled, additionally the gocyclo linter with ccn > 10. Signed-off-by: Daniel Hiller <[email protected]>

Signed-off-by: Daniel Hiller <[email protected]>

Adds a `test` target to the Makefile that executes the go tests. Signed-off-by: Daniel Hiller <[email protected]>

Adds unit tests for sig validation as precondition of refactoring. Signed-off-by: Daniel Hiller <[email protected]>

* reduce complexity by converting all groups into a single map * replace if then with early return * extract validateLeads, use that in two places Signed-off-by: Daniel Hiller <[email protected]>

Signed-off-by: Daniel Hiller <[email protected]>

dhiller · 2025-01-16T16:58:46Z

@lyarwood thank you for the review and apologies for the long wait time. I have addressed your comments, but since a rebase happened some linting errors had entered the code base again. I have fixed these now.

PTAL, thank you! 🙏

lyarwood

/lgtm

dhiller · 2025-01-17T11:18:07Z

🏓 @aburdenthehand 🙂

aburdenthehand · 2025-01-17T14:31:29Z

@xpivarc - you happy?

kubevirt-bot requested review from aburdenthehand and jean-edouard December 11, 2024 17:40

kubevirt-bot added the size/XL label Dec 11, 2024

dhiller force-pushed the cleanup-code branch from ab42c5f to 4471b16 Compare December 12, 2024 12:54

dhiller marked this pull request as ready for review December 12, 2024 13:28

kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 12, 2024

dhiller mentioned this pull request Dec 12, 2024

community: add test, lint and coverage jobs kubevirt/project-infra#3824

Open

kubevirt-bot requested a review from EdDev December 16, 2024 09:20

xpivarc reviewed Jan 7, 2025

View reviewed changes

kubevirt-bot requested a review from lyarwood January 9, 2025 10:30

lyarwood reviewed Jan 15, 2025

View reviewed changes

dhiller added 8 commits January 16, 2025 15:03

lint: add make target

c8729eb

Adds a make target `lint` that will be used to run enabled linters, which are configured via .golangci.yaml . Initially only the default linters are enabled, additionally the gocyclo linter with ccn > 10. Signed-off-by: Daniel Hiller <[email protected]>

lint: fix deprecated io/ioutil usage

5d04c82

Signed-off-by: Daniel Hiller <[email protected]>

sigs-validator,refactor: extract funcs to reduce ccn

8a5575e

Signed-off-by: Daniel Hiller <[email protected]>

go,test: add make target

612090f

Adds a `test` target to the Makefile that executes the go tests. Signed-off-by: Daniel Hiller <[email protected]>

sigs-validator: add unit tests

8477cf7

Adds unit tests for sig validation as precondition of refactoring. Signed-off-by: Daniel Hiller <[email protected]>

cleanup,refactor: sigs-validator

ebc8371

* reduce complexity by converting all groups into a single map * replace if then with early return * extract validateLeads, use that in two places Signed-off-by: Daniel Hiller <[email protected]>

refactor,repo_groups: extract func

faa0a73

Signed-off-by: Daniel Hiller <[email protected]>

coverage: add make target

e1cb659

Signed-off-by: Daniel Hiller <[email protected]>

dhiller force-pushed the cleanup-code branch from b62b0e6 to d2504f8 Compare January 16, 2025 15:28

review: fix issues

06e39c8

Signed-off-by: Daniel Hiller <[email protected]>

dhiller force-pushed the cleanup-code branch from d2504f8 to 06e39c8 Compare January 16, 2025 15:33

dhiller requested a review from lyarwood January 16, 2025 15:34

dhiller added 2 commits January 16, 2025 17:40

chore: fix linting errors

5675627

Signed-off-by: Daniel Hiller <[email protected]>

chore,refactoring: community report

1419600

Signed-off-by: Daniel Hiller <[email protected]>

lyarwood approved these changes Jan 16, 2025

View reviewed changes

kubevirt-bot assigned lyarwood Jan 16, 2025

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

community: cleanup code, reduce complexity, add make targets #368

community: cleanup code, reduce complexity, add make targets #368

dhiller commented Dec 11, 2024 •

edited

Loading

kubevirt-bot commented Dec 11, 2024

kubevirt-bot commented Dec 11, 2024

aburdenthehand commented Dec 16, 2024

kubevirt-bot commented Dec 16, 2024

xpivarc Jan 7, 2025

dhiller Jan 9, 2025

dhiller Jan 10, 2025

xpivarc Jan 10, 2025

dhiller Jan 10, 2025

dhiller Jan 10, 2025

lyarwood commented Jan 9, 2025

lyarwood Jan 15, 2025

dhiller Jan 15, 2025

lyarwood Jan 15, 2025

lyarwood Jan 15, 2025

dhiller commented Jan 16, 2025

lyarwood left a comment

dhiller commented Jan 17, 2025

aburdenthehand commented Jan 17, 2025

community: cleanup code, reduce complexity, add make targets #368

Are you sure you want to change the base?

community: cleanup code, reduce complexity, add make targets #368

Conversation

dhiller commented Dec 11, 2024 • edited Loading

kubevirt-bot commented Dec 11, 2024

kubevirt-bot commented Dec 11, 2024

aburdenthehand commented Dec 16, 2024

kubevirt-bot commented Dec 16, 2024

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

lyarwood commented Jan 9, 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

dhiller commented Jan 16, 2025

lyarwood left a comment

Choose a reason for hiding this comment

dhiller commented Jan 17, 2025

aburdenthehand commented Jan 17, 2025

dhiller commented Dec 11, 2024 •

edited

Loading