Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🌱 add a verify rule for golang files import order #177

Merged

Conversation

zhujian7
Copy link
Member

@zhujian7 zhujian7 commented Jun 8, 2023

Summary

This PR uses the gci tool to make all go files' import section with a specific order, it will organize import with group with order:

  1. standard library modules
  2. 3rd party modules
  3. modules in OCM org, like the open-cluster-management.io/api
  4. current project open-cluster-management.io/ocm modules

developers can use the make fmt-imports to format the import automatically and the make verify-fmt-imports to check for any violation.

Related issue(s)

Fixes #

@openshift-ci openshift-ci bot requested review from deads2k and mikeshng June 8, 2023 11:08
@codecov
Copy link

codecov bot commented Jun 8, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (3c9bfea) 58.36% compared to head (313842b) 58.36%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #177   +/-   ##
=======================================
  Coverage   58.36%   58.36%           
=======================================
  Files         109      109           
  Lines       11713    11713           
=======================================
  Hits         6836     6836           
  Misses       4212     4212           
  Partials      665      665           
Flag Coverage Δ
unit 58.36% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/common/patcher/patcher.go 70.32% <ø> (ø)
pkg/operator/certrotation/cabundle.go 46.87% <ø> (ø)
pkg/operator/certrotation/target.go 58.22% <ø> (ø)
pkg/operator/helpers/helpers.go 67.41% <ø> (ø)
pkg/operator/helpers/queuekey.go 69.60% <ø> (ø)
.../certrotationcontroller/certrotation_controller.go 66.20% <ø> (ø)
...stermanagercontroller/clustermanager_controller.go 54.27% <ø> (ø)
...rmanagercontroller/clustermanager_crd_reconcile.go 60.37% <ø> (ø)
...rmanagercontroller/clustermanager_hub_reconcile.go 50.74% <ø> (ø)
...agercontroller/clustermanager_runtime_reconcile.go 54.05% <ø> (ø)
... and 89 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@xuezhaojun
Copy link
Member

xuezhaojun commented Jun 8, 2023

Maybe we can import this indirectly by depending on golangci-lint https://golangci-lint.run/usage/linters/#gci, it's like a more commonly used linter tool integrated with more features including gci.

Also, the developers may have different fmt tools on their laptops, if we want to add a verification step to the workflow, it would become a potential standard and they may need to configure their editors to use the same tool to fmt go files.

@zhujian7
Copy link
Member Author

zhujian7 commented Jun 8, 2023

Maybe we can import this indirectly by depending on golangci-lint https://golangci-lint.run/usage/linters/#gci, it's like a more commonly used linter tool integrated with more features including gci.

Thanks @xuezhaojun yes, I have considered using golangci-lint, but once enable the lint check, there are so many errors that need to fix, which makes the PR hard to review, so I will add the lint check in another PR.

Makefile Outdated Show resolved Hide resolved
else \
echo "Diff output is empty"; \
fi

Copy link
Member

Choose a reason for hiding this comment

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

this could be put under verify target, so you do not need create another action step

Copy link
Member Author

Choose a reason for hiding this comment

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

after this PR is merged, I will integrate the verification into golangci-lint: https://golangci-lint.run/usage/linters/#gci by a follow up PR

@zhujian7
Copy link
Member Author

zhujian7 commented Jun 9, 2023

/hold

@qiujian16
Copy link
Member

/approve
/lgtm

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 9, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qiujian16, zhujian7

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:

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

@openshift-ci openshift-ci bot added the approved label Jun 9, 2023
zhujian7 added 2 commits June 12, 2023 09:18
This PR uses the [gci tool](https://github.com/daixiang0/gci) to make all go files' import section with a specific order, it will organize import with group with order:
1. standard library modules
2. 3rd party modules
3. modules in OCM org, like the `open-cluster-management.io/api`
4. current project `open-cluster-management.io/ocm` modules

developers can use the `make fmt-imports` to format the import automatically and the `make verify-fmt-imports` to check for any violation.

Signed-off-by: zhujian <jiazhu@redhat.com>
Signed-off-by: zhujian <jiazhu@redhat.com>
@qiujian16
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jun 12, 2023
@zhujian7
Copy link
Member Author

/unhold

@openshift-merge-robot openshift-merge-robot merged commit 7332a58 into open-cluster-management-io:main Jun 12, 2023
ycyaoxdu pushed a commit to ycyaoxdu/ocm that referenced this pull request Jun 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants