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

Use golangci-lint as linter for Go #1671

Merged

Conversation

tenzen-y
Copy link
Member

@tenzen-y tenzen-y commented Sep 21, 2021

What this PR does / why we need it:

  • Replace golint to golangci-lint.

If we enable all default items in golangci-lint, we will receive a lot of lint errors. Although, I think It might be better to fix all lint errors.
What do you think? @andreyvelich @gaocegege @johnugeorge

Ref:

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 #1670

@aws-kf-ci-bot
Copy link
Contributor

Hi @tenzen-y. 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.

@tenzen-y
Copy link
Member Author

/assign
/cc @andreyvelich @johnugeorge

Copy link
Member

@andreyvelich andreyvelich left a comment

Choose a reason for hiding this comment

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

Thank you for taking this @tenzen-y!

Although, I think It might be better to fix all lint errors.

Yes, it would be great if we could fix them in this PR.

hack/verify-golangci-lint.sh Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
hack/verify-golangci-lint.sh Outdated Show resolved Hide resolved
@andreyvelich
Copy link
Member

/ok-to-test

@gaocegege
Copy link
Member

If we enable all default items in golangci-lint, we will receive a lot of lint errors. Although, I think It might be better to fix all lint errors.

I have no objection to it.

tenzen-y and others added 4 commits September 22, 2021 14:01
Co-authored-by: Andrey Velichkevich <andrey.velichkevich@gmail.com>
Co-authored-by: Andrey Velichkevich <andrey.velichkevich@gmail.com>
Co-authored-by: Andrey Velichkevich <andrey.velichkevich@gmail.com>
Co-authored-by: Andrey Velichkevich <andrey.velichkevich@gmail.com>
@tenzen-y
Copy link
Member Author

Thank you for your comment! @andreyvelich @gaocegege
I'd like to fix all lint errors in this PR!

/hold

@tenzen-y tenzen-y changed the title Use golangci-lint as linter for Go WIP: Use golangci-lint as linter for Go Sep 22, 2021
@tenzen-y tenzen-y changed the title WIP: Use golangci-lint as linter for Go [WIP] Use golangci-lint as linter for Go Sep 22, 2021
@andreyvelich
Copy link
Member

Maybe, I guess fixed the following issues without separating test managers in this PR.

You think handling some errors can help with it ?

@tenzen-y
Copy link
Member Author

Maybe, I guess fixed the following issues without separating test managers in this PR.

You think handling some errors can help with it ?

As discussed in #1649, I also think the tests were sometimes failing due to the heavy load on the manager.
In this PR, I fixed the unnecessary repetition of resource creation and deletion. As a result, the load on the manager has been reduced and crashes have become less frequent. In the future, as more test code is added, the possibility of another crash will increase.

tenzen-y and others added 3 commits September 23, 2021 07:21
Co-authored-by: Andrey Velichkevich <andrey.velichkevich@gmail.com>
Co-authored-by: Andrey Velichkevich <andrey.velichkevich@gmail.com>
Co-authored-by: Andrey Velichkevich <andrey.velichkevich@gmail.com>
@google-cla
Copy link

google-cla bot commented Sep 22, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@tenzen-y
Copy link
Member Author

@googlebot I consent.

@google-cla
Copy link

google-cla bot commented Sep 22, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@tenzen-y
Copy link
Member Author

@andreyvelich
Could you comment @googlebot I consent., please?

@google-cla
Copy link

google-cla bot commented Sep 22, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@tenzen-y
Copy link
Member Author

/retest

@andreyvelich
Copy link
Member

As a result, the load on the manager has been reduced and crashes have become less frequent. In the future, as more test code is added, the possibility of another crash will increase

Makes sense, let's see if it helps.

@andreyvelich
Copy link
Member

@googlebot I consent.

@tenzen-y
Copy link
Member Author

/unhold

@andreyvelich
Copy link
Member

Thank you for doing this @tenzen-y!
/lgtm
/assign @gaocegege @johnugeorge

@gaocegege
Copy link
Member

@tenzen-y
Copy link
Member Author

tenzen-y commented Sep 24, 2021

There are some lint errors here https://github.com/kubeflow/katib/pull/1671/checks?check_run_id=3671380935

Thanks for the review! @gaocegege
I think it has been resolved in the latest test.

https://github.com/kubeflow/katib/pull/1671/checks?check_run_id=3680981981#step:5:1502

@gaocegege
Copy link
Member

/lgtm
/approve

Thanks for your contribution! 🎉 👍

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gaocegege, tenzen-y

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

@google-oss-robot google-oss-robot merged commit c22afe9 into kubeflow:master Sep 24, 2021
@tenzen-y tenzen-y deleted the issue-1670-add-golangci-lint branch September 24, 2021 06:29
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.

golint has been archived
6 participants