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.

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

hack: make golangci-lint.sh more verbose #1718

Merged
merged 3 commits into from
Aug 16, 2022
Merged

hack: make golangci-lint.sh more verbose #1718

merged 3 commits into from
Aug 16, 2022

Conversation

jarrpa
Copy link
Member

@jarrpa jarrpa commented Jun 16, 2022

This is to help debug problems when running in the OpenShift CI, but
also should be generally useful.

Signed-off-by: Jose A. Rivera [email protected]

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 16, 2022
@jarrpa
Copy link
Member Author

jarrpa commented Jun 16, 2022

/test ocs-operator-ci

@jarrpa
Copy link
Member Author

jarrpa commented Jun 16, 2022

/override ci/prow/red-hat-storage-ocs-ci-e2e-aws

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 16, 2022

@jarrpa: Overrode contexts on behalf of jarrpa: ci/prow/red-hat-storage-ocs-ci-e2e-aws

In response to this:

/override ci/prow/red-hat-storage-ocs-ci-e2e-aws

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.

@jarrpa
Copy link
Member Author

jarrpa commented Jun 16, 2022

/retest

@jarrpa
Copy link
Member Author

jarrpa commented Jun 16, 2022

/test ocs-operator-bundle-e2e-aws

iamniting
iamniting requested changes Jun 17, 2022
View reviewed changes
Copy link
Member

@iamniting iamniting left a comment

Choose a reason for hiding this comment

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

Instead of this curl and script method why don't we directly use go install

@jarrpa
Copy link
Member Author

jarrpa commented Jun 17, 2022

Instead of this curl and script method why don't we directly use go install

Because these are the recommended instructions, after just using GH Actions, per their own documentation: https://golangci-lint.run/usage/install/ This also allows devs to run it locally without pushing to a repo first.

@iamniting
Copy link
Member

iamniting commented Jun 17, 2022 •
edited
Loading

Instead of this curl and script method why don't we directly use go install

Because these are the recommended instructions, after just using GH Actions, per their own documentation: https://golangci-lint.run/usage/install/ This also allows devs to run it locally without pushing to a repo first.

Even installing via go install allows the same as we are doing other installs like controller-gen etc in the makefile.

https://golangci-lint.run/usage/install/#install-from-source

@jarrpa
Copy link
Member Author

jarrpa commented Jun 17, 2022

Instead of this curl and script method why don't we directly use go install

Because these are the recommended instructions, after just using GH Actions, per their own documentation: https://golangci-lint.run/usage/install/ This also allows devs to run it locally without pushing to a repo first.

Even installing via go install allows the same as we are doing other installs like controller-gen etc in the makefile.

https://golangci-lint.run/usage/install/#install-from-source

Quoting from that URL ;)

Install from Source

Note: such go install/go get installation aren't guaranteed to work. We recommend using binary installation.
Why?

go install/go get installation isn't recommended because of the following points:

1.    some users use -u flag for go get, which upgrades our dependencies. Resulting configuration wasn't tested and isn't guaranteed to work.
2.    [go.mod](https://github.com/golangci/golangci-lint/blob/master/go.mod) replacement directive doesn't apply. It means a user will be using patched version of golangci-lint if we use such replacements.
3.    it's stability depends on a user's Go version (e.g. on [this compiler Go <= 1.12 bug](https://github.com/golang/go/issues/29612)).
4,    we've encountered a lot of issues with Go modules hashes.
5.    it allows installation from master branch which can't be considered stable.
6.    it's slower than binary installation

@jarrpa
Copy link
Member Author

jarrpa commented Jun 17, 2022

Unrelated, the CI still seems to be facing some networking issues, this time in functests.... I'll keep an eye on this and bug the DPTP team as needed.

iamniting
iamniting approved these changes Jun 17, 2022
View reviewed changes
Copy link
Member

@iamniting iamniting 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. and removed lgtm Indicates that a PR is ready to be merged. labels Jun 17, 2022
@jarrpa
Copy link
Member Author

jarrpa commented Jun 17, 2022

I added a new commit for build-functests, just to see.

@jarrpa
Copy link
Member Author

jarrpa commented Jun 17, 2022

Yup, problem still persists... let's keep this open and retesting until we confirm the issue has been resolved.

/hold

@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 17, 2022
@iamniting iamniting mentioned this pull request Jun 17, 2022
Closed
@jarrpa
Copy link
Member Author

jarrpa commented Jun 17, 2022

/override ci/prow/red-hat-storage-ocs-ci-e2e-aws

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 17, 2022

@jarrpa: Overrode contexts on behalf of jarrpa: ci/prow/red-hat-storage-ocs-ci-e2e-aws

In response to this:

/override ci/prow/red-hat-storage-ocs-ci-e2e-aws

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.

@jarrpa
Copy link
Member Author

jarrpa commented Jun 17, 2022

/retest

1 similar comment
@jarrpa
Copy link
Member Author

jarrpa commented Jun 17, 2022

/retest

@jarrpa
Copy link
Member Author

jarrpa commented Jun 17, 2022

/test ocs-operator-ci

2 similar comments
@jarrpa
Copy link
Member Author

jarrpa commented Jun 18, 2022

/test ocs-operator-ci

@jarrpa
Copy link
Member Author

jarrpa commented Jun 21, 2022

/test ocs-operator-ci

@jarrpa jarrpa force-pushed the verbose-golangci-script branch 2 times, most recently from badd44d to d9cf9e6 Compare June 22, 2022 15:21
@jarrpa
Copy link
Member Author

jarrpa commented Jun 28, 2022

/test ocs-operator-ci

@jarrpa
hack: replace deprecated test in golangci-lint
6ccaf39 
Signed-off-by: Jose A. Rivera <[email protected]>
@jarrpa
hack: code fixes from linters
b3be31d 
Signed-off-by: Jose A. Rivera <[email protected]>
@jarrpa
hack: make golangci-lint.sh more verbose
9d84dee 
This is to work around problems when running in the OpenShift CI, but
should also be more robust and generally useful.

Signed-off-by: Jose A. Rivera <[email protected]>
@jarrpa
Copy link
Member Author

jarrpa commented Aug 15, 2022

/override ci/prow/red-hat-storage-ocs-ci-e2e-aws

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 15, 2022

@jarrpa: Overrode contexts on behalf of jarrpa: ci/prow/red-hat-storage-ocs-ci-e2e-aws

In response to this:

/override ci/prow/red-hat-storage-ocs-ci-e2e-aws

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.

@jarrpa
Copy link
Member Author

jarrpa commented Aug 15, 2022

/test ocs-operator-bundle-e2e-aws

@jarrpa
Copy link
Member Author

jarrpa commented Aug 16, 2022

/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 Aug 16, 2022
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 16, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 16, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BlaineEXE, iamniting, jarrpa

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-merge-robot openshift-merge-robot merged commit a81f5ce into red-hat-storage:main Aug 16, 2022
@jarrpa jarrpa deleted the verbose-golangci-script branch September 1, 2022 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BlaineEXE BlaineEXE BlaineEXE approved these changes

@iamniting iamniting iamniting approved these changes

Assignees

@BlaineEXE BlaineEXE

@iamniting iamniting

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
@jarrpa @iamniting @BlaineEXE @openshift-merge-robot