chore: Enable go fmt as a lint check for Go code #11830

cmdevoto · 2025-04-15T15:26:16Z

signed off by: Caroline DeVoto cmdevoto@users.noreply.github.com
Resolves #11825

Description of your changes:
Added golangci-lint fmt a pre commit hook with gofmt and goimports enabled to ensure standard format across Go source files. The .golangci.yaml file was also refactored for version 2.

Note that the pycln version is updated as well in the .pre-commit-config.yaml due to an issue with Python >=3.12 that was fixed in the v2.2.0 release.

Checklist:

You have signed off your commits
The title for your pull request (PR) should follow our title convention. Learn more about the pull request title convention used in this repository.

google-oss-prow · 2025-04-15T15:26:27Z

Hi @cmdevoto. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

github-actions · 2025-04-15T15:27:00Z

🚫 This command cannot be processed. Only organization members or owners can use the commands.

cmdevoto · 2025-04-15T15:42:35Z

.pre-commit-config.yaml

@@ -65,3 +65,11 @@ repos:
        language: golang
        require_serial: true
        pass_filenames: false
+  - repo: local


Happy to change this to use a repo like https://github.com/Bahjat/pre-commit-golang or https://github.com/dnephin/pre-commit-golang

zazulam · 2025-04-15T16:30:15Z

/ok-to-test

zazulam · 2025-04-15T20:52:18Z

love to see standardizing formatting
/lgtm

droctothorpe · 2025-04-22T14:52:38Z

...ient/experiment_client/experiment_service/experiment_service_get_experiment_v1_parameters.go

@@ -56,7 +56,8 @@ func NewExperimentServiceGetExperimentV1ParamsWithHTTPClient(client *http.Client
 	}
 }

-/*ExperimentServiceGetExperimentV1Params contains all the parameters to send to the API endpoint
+/*


This file and many others modified in this PR are generated (see this for example).

@HumairAK @mprahl do you have a preference on whether or not to exclude generated files from go fmt?

@droctothorpe thanks for pointing this out. Yes, I agree that we should exclude generated files that developers are never expected to directly modify.

Thanks for the feedback, @droctothorpe & @mprahl! All of the files modified by go fmt ./... were in backend/api/v1beta1/go_http_client and backend/api/v2beta1/go_http_client, which all seem to be autogenerated files. Do you still want the pre-commit hook?

@cmdevoto how about also adding goimports?

Thanks for the feedback, @droctothorpe & @mprahl! All of the files modified by go fmt ./... were in backend/api/v1beta1/go_http_client and backend/api/v2beta1/go_http_client, which all seem to be autogenerated files. Do you still want the pre-commit hook?

IMO, yes. It will enforce standardized formatting for future, non-generated golang contributions.

@droctothorpe @mprahl Version 2 of golangci-lint introduced formatting with golangci-lint fmt(along with some other configuration file changes). Thoughts on adding gofmt and goimports to the config file and just adding another pre-commit hook to run the format command?

@droctothorpe @mprahl Hi all! Made the changes as described in my comment above - let me know what you think

cmdevoto · 2025-04-30T21:46:10Z

.golangci.yaml

    - govet
    - ineffassign
    - misspell
    - staticcheck
-    - stylecheck


removed as gosimple and stylecheck were removed in version 2

mprahl · 2025-05-01T15:00:41Z

.pre-commit-config.yaml

@@ -65,3 +65,11 @@ repos:
        language: golang
        require_serial: true
        pass_filenames: false
+      - id: golangci-lint


Why are there two entries for golangci-lint?

Because each one has a different entry: golangci-lint run --new-from-rev HEAD --fix and golangci-lint fmt. I think you can have multiple entries for one hook ID but then you wouldn't be able to tell which command failed.

I may be misunderstanding, but wouldn't the second entry be more comprehensive than the first and thus making the first unnecessary?

I believe that golangci-lint run runs the linters while golangci-lint fmt runs the formatters

Got it, makes sense! I missed the run vs fmt distinction.

mprahl · 2025-05-01T15:02:03Z

.pre-commit-config.yaml

@@ -65,3 +65,11 @@ repos:
        language: golang
        require_serial: true
        pass_filenames: false
+      - id: golangci-lint


I think it'd also be nice to have a Make target to run linting as well.

Good idea, added in latest commit 👍

mprahl · 2025-05-12T15:33:10Z

@cmdevoto where is this enforced in the PR CI?

cmdevoto · 2025-05-12T17:38:29Z

@cmdevoto where is this enforced in the PR CI?

cc @mprahl Do you mean as part of the github workflows? I don't believe any of the pre-commit hooks are enforced in a PR's CI currently (but correct me if I'm wrong). Do you want just the golangci-lint as part of the CI or all of the pre-commit hooks? If yes to either, it might make sense to add that as part of a separate PR that would precede this one.

mprahl · 2025-05-12T20:51:18Z

@cmdevoto where is this enforced in the PR CI?

cc @mprahl Do you mean as part of the github workflows? I don't believe any of the pre-commit hooks are enforced in a PR's CI currently (but correct me if I'm wrong). Do you want just the golangci-lint as part of the CI or all of the pre-commit hooks? If yes to either, it might make sense to add that as part of a separate PR that would precede this one.

If you want to start with just golangci-lint for now and include it in this PR, then I think that'd be fine.

cmdevoto · 2025-05-13T02:58:48Z

@cmdevoto where is this enforced in the PR CI?

cc @mprahl Do you mean as part of the github workflows? I don't believe any of the pre-commit hooks are enforced in a PR's CI currently (but correct me if I'm wrong). Do you want just the golangci-lint as part of the CI or all of the pre-commit hooks? If yes to either, it might make sense to add that as part of a separate PR that would precede this one.

If you want to start with just golangci-lint for now and include it in this PR, then I think that'd be fine.

@mprahl I added a new github workflow that only runs the golangci-lint pre-commit hooks currently. Here are the passing checks in the PR CI.

mprahl · 2025-05-14T14:24:59Z

/rerun-workflow "KFP e2e tests"

mprahl · 2025-05-14T14:25:10Z

/rerun-workflow "CI Check"

Signed-off-by: Caroline DeVoto <cmdevoto@users.noreply.github.com>

zazulam · 2025-05-16T13:33:27Z

/rerun-workflow "CI Check"

mprahl

/lgtm
/approve

HumairAK · 2025-05-19T14:30:41Z

/approve

clean code ftw, thanks team!

google-oss-prow · 2025-05-19T14:30:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: HumairAK, mprahl

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~~ [HumairAK]

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

google-oss-prow bot requested review from gmfrasca and HumairAK April 15, 2025 15:26

google-oss-prow bot added the needs-ok-to-test label Apr 15, 2025

google-oss-prow bot added the size/XXL label Apr 15, 2025

cmdevoto force-pushed the gofmt-changes branch from b34ebba to d593b7a Compare April 15, 2025 15:29

cmdevoto commented Apr 15, 2025

View reviewed changes

cmdevoto force-pushed the gofmt-changes branch from d593b7a to 2ad4a18 Compare April 15, 2025 15:52

google-oss-prow bot added ok-to-test and removed needs-ok-to-test labels Apr 15, 2025

google-oss-prow bot assigned zazulam Apr 15, 2025

google-oss-prow bot added the lgtm label Apr 15, 2025

droctothorpe reviewed Apr 22, 2025

View reviewed changes

cmdevoto force-pushed the gofmt-changes branch from 2ad4a18 to 75ae1f3 Compare April 30, 2025 21:45

google-oss-prow bot added size/L and removed lgtm size/XXL labels Apr 30, 2025

cmdevoto commented Apr 30, 2025

View reviewed changes

mprahl reviewed May 1, 2025

View reviewed changes

HumairAK added this to KFP Project Tracker May 7, 2025

HumairAK moved this to In Review in KFP Project Tracker May 7, 2025

HumairAK moved this from In Review to In Progress in KFP Project Tracker May 7, 2025

cmdevoto force-pushed the gofmt-changes branch from 75ae1f3 to d59e82d Compare May 8, 2025 02:10

cmdevoto requested a review from mprahl May 12, 2025 15:20

cmdevoto force-pushed the gofmt-changes branch 3 times, most recently from 7e7a991 to 7dc5621 Compare May 13, 2025 02:50

cmdevoto force-pushed the gofmt-changes branch from 7451114 to 89a9332 Compare May 15, 2025 18:32

chore: enable golangci-lint fmt

89a9332

Signed-off-by: Caroline DeVoto <cmdevoto@users.noreply.github.com>

mprahl approved these changes May 16, 2025

View reviewed changes

google-oss-prow bot assigned mprahl May 16, 2025

google-oss-prow bot added the lgtm label May 16, 2025

google-oss-prow bot added the approved label May 19, 2025

google-oss-prow bot merged commit e329fa3 into kubeflow:master May 19, 2025
45 of 46 checks passed

github-project-automation bot moved this from In Progress to Done in KFP Project Tracker May 19, 2025

This was referenced May 21, 2025

cmdevoto membership request kubeflow/internal-acls#784

Closed

chore: add cmdevoto to members org kubeflow/internal-acls#785

Merged

chore: Enable go fmt as a lint check for Go code #11830

chore: Enable go fmt as a lint check for Go code #11830

Uh oh!

Conversation

cmdevoto commented Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

google-oss-prow bot commented Apr 15, 2025

Uh oh!

github-actions bot commented Apr 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zazulam commented Apr 15, 2025

Uh oh!

zazulam commented Apr 15, 2025

Uh oh!

droctothorpe Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmdevoto Apr 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmdevoto May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mprahl commented May 12, 2025

Uh oh!

cmdevoto commented May 12, 2025

Uh oh!

mprahl commented May 12, 2025

Uh oh!

cmdevoto commented May 13, 2025

Uh oh!

mprahl commented May 14, 2025

Uh oh!

mprahl commented May 14, 2025

Uh oh!

zazulam commented May 16, 2025

Uh oh!

mprahl left a comment

Choose a reason for hiding this comment

Uh oh!

HumairAK commented May 19, 2025

Uh oh!

google-oss-prow bot commented May 19, 2025

Uh oh!

Uh oh!

Uh oh!

cmdevoto commented Apr 15, 2025 •

edited

Loading

droctothorpe Apr 22, 2025 •

edited

Loading

cmdevoto Apr 23, 2025 •

edited

Loading

cmdevoto May 1, 2025 •

edited

Loading