Skip to content

Enable staticcheck #604

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.

Sign up for GitHub

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

Jump to bottom
Merged
openshift-merge-robot merged 1 commit into from
Jun 25, 2021
Merged

Enable staticcheck #604

openshift-merge-robot merged 1 commit into from
Jun 25, 2021

Conversation

@arjunrn
Copy link
Contributor

@arjunrn arjunrn commented Jun 14, 2021 •
edited
Loading

Depends on #598 and #602

staticcheck is a code analysis tool which detects common mistakes in go.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 14, 2021
@arjunrn arjunrn force-pushed the enable-staticcheck branch from ea8c018 to 8852bb7 Compare June 17, 2021 09:31
@arjunrn
Copy link
Contributor Author

arjunrn commented Jun 17, 2021

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 17, 2021
arjunrn
arjunrn commented Jun 17, 2021
View reviewed changes
cmd/image.go
klog.Fatalf("error: %v", err)
}
fmt.Printf(image)
fmt.Print(image)
Copy link
Contributor Author

@arjunrn arjunrn Jun 17, 2021

Choose a reason for hiding this comment

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

There is no formatting of the strings required here.

arjunrn
arjunrn commented Jun 17, 2021
View reviewed changes
pkg/cvo/egress.go
return nil, err
}

if &proxy.Spec != nil {
Copy link
Contributor Author

@arjunrn arjunrn Jun 17, 2021

Choose a reason for hiding this comment

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

proxy is referenced and compared to nil. Since proxy is a value type this check will never be false.

Copy link

@vrutkovs vrutkovs Jun 23, 2021

Choose a reason for hiding this comment

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

Looks good, but I'm not entirely sure if these are equivalent.

/cc @jottofar @wking

Copy link
Contributor Author

@arjunrn arjunrn Jun 23, 2021

Choose a reason for hiding this comment

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

I didn't change the check. I removed it and what you see now is the inner-check. The outer-check was superfluous for the reason mentioned above.

@arjunrn
Copy link
Contributor Author

arjunrn commented Jun 18, 2021

/assign @LalatenduMohanty

@vrutkovs
Copy link

vrutkovs commented Jun 22, 2021

/retest

@openshift-ci openshift-ci bot requested review from jottofar and wking June 23, 2021 10:20
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 23, 2021
LalatenduMohanty
LalatenduMohanty approved these changes Jun 23, 2021
View reviewed changes
Copy link
Member

@LalatenduMohanty LalatenduMohanty left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 23, 2021
@arjunrn arjunrn force-pushed the enable-staticcheck branch from 8852bb7 to 90767ce Compare June 24, 2021 09:57
@openshift-ci openshift-ci 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 Jun 24, 2021
@vrutkovs
Copy link

vrutkovs commented Jun 25, 2021

/override ci/prow/e2e-agnostic-upgrade
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 25, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 25, 2021

@vrutkovs: Overrode contexts on behalf of vrutkovs: ci/prow/e2e-agnostic-upgrade

Details

In response to this:

/override ci/prow/e2e-agnostic-upgrade
/lgtm

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.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 25, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arjunrn, LalatenduMohanty, vrutkovs

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

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [LalatenduMohanty,vrutkovs]

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

@openshift-merge-robot openshift-merge-robot merged commit bfa568e into openshift:master Jun 25, 2021
@arjunrn arjunrn deleted the enable-staticcheck branch June 25, 2021 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sdodson sdodson Awaiting requested review from sdodson

@jottofar jottofar Awaiting requested review from jottofar

@wking wking Awaiting requested review from wking

2 more reviewers

@vrutkovs vrutkovs vrutkovs left review comments

@LalatenduMohanty LalatenduMohanty LalatenduMohanty approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@vrutkovs vrutkovs

@LalatenduMohanty LalatenduMohanty

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@arjunrn @vrutkovs @LalatenduMohanty @openshift-merge-robot