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

[3.4] installed revive on CI and added doc.go to fix revive linter errors #18215

Merged
merged 2 commits into from
Jul 4, 2024

Conversation

thedtripp
Copy link
Member

@thedtripp thedtripp commented Jun 22, 2024

This is meant to address the failing revive checks mentioned in this issue: #17472

Screenshot of Revive linter errors:
image

Output with code changes:
image

@k8s-ci-robot
Copy link

Hi @thedtripp. Thanks for your PR.

I'm waiting for a etcd-io 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-sigs/prow repository.

@thedtripp
Copy link
Member Author

What are your thoughts on this PR @jmhbnz?

@ivanvc
Copy link
Member

ivanvc commented Jun 27, 2024

This looks good overall. I wonder if a better approach would be to add a doc.go file to the rest of the packages that don't have the module documentation. However, I also don't know if that's something we want to do for 3.4 or if we should go with the current approach. Thoughts, @jmhbnz?

@ivanvc
Copy link
Member

ivanvc commented Jun 27, 2024

/ok-to-test

@ivanvc
Copy link
Member

ivanvc commented Jun 27, 2024

/retitle [3.4] formatting: added package comments to fix revive linter errors.

@k8s-ci-robot k8s-ci-robot changed the title ectd 3.4 formatting: added package comments to fix revive linter errors. [3.4] formatting: added package comments to fix revive linter errors. Jun 27, 2024
@jmhbnz
Copy link
Member

jmhbnz commented Jun 29, 2024

This looks good overall. I wonder if a better approach would be to add a doc.go file to the rest of the packages that don't have the module documentation. However, I also don't know if that's something we want to do for 3.4 or if we should go with the current approach. Thoughts, @jmhbnz?

This decision I think is best to be made by @ahrtr and/or @serathius who were around for much more of 3.4 development than I was. Personally I would lean towards adding missing doc.go files but that might be overkill.

@ahrtr
Copy link
Member

ahrtr commented Jun 29, 2024

#17472 (comment)

Please ensure the linter isn't skipped firstly, then resolve the errors detected by the linter.

Personally I would lean towards adding missing doc.go files

OK for me. Actually most of the packages already have doc.go files.

@thedtripp
Copy link
Member Author

Please ensure the linter isn't skipped firstly, then resolve the errors detected by the linter.

@ahrtr Thanks for the review. Should this be done in one PR, or two separate ones?

@ahrtr
Copy link
Member

ahrtr commented Jun 29, 2024

@ahrtr Thanks for the review. Should this be done in one PR, or two separate ones?

Please do it in one PR, you can have one or two commits (the first commit ensures the linter isn't skipped; the second commit fixes the error raised by the linter).

@thedtripp thedtripp force-pushed the feature/fixFailingReviveChecks branch from 4f15506 to 83c1c4c Compare June 30, 2024 03:16
@thedtripp thedtripp force-pushed the feature/fixFailingReviveChecks branch from 29d05f7 to 731d92c Compare June 30, 2024 05:13
@thedtripp thedtripp force-pushed the feature/fixFailingReviveChecks branch from 731d92c to 29d05f7 Compare June 30, 2024 05:18
@thedtripp thedtripp force-pushed the feature/fixFailingReviveChecks branch 3 times, most recently from 7e4a399 to b0d4ea2 Compare June 30, 2024 05:40
@thedtripp
Copy link
Member Author

/retitle [3.4] installed revive on CI and added doc.go to fix revive linter errors.

@k8s-ci-robot
Copy link

@thedtripp: Re-titling can only be requested by trusted users, like repository collaborators.

In response to this:

/retitle [3.4] installed revive on CI and added doc.go to fix revive linter errors.

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-sigs/prow repository.

@jmhbnz
Copy link
Member

jmhbnz commented Jun 30, 2024

/retitle [3.4] installed revive on CI and added doc.go to fix revive linter errors

@k8s-ci-robot k8s-ci-robot changed the title [3.4] formatting: added package comments to fix revive linter errors. [3.4] installed revive on CI and added doc.go to fix revive linter errors Jun 30, 2024
@thedtripp thedtripp force-pushed the feature/fixFailingReviveChecks branch from b0d4ea2 to 692822d Compare June 30, 2024 20:02
@thedtripp
Copy link
Member Author

/joke

@k8s-ci-robot
Copy link

@thedtripp: How can you tell a vampire has a cold? They start coffin.

In response to this:

/joke

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-sigs/prow repository.

@thedtripp
Copy link
Member Author

/cc @jmhbnz @ivanvc @ahrtr

This PR is ready for review. 🚀

Copy link
Member

@ivanvc ivanvc left a comment

Choose a reason for hiding this comment

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

This looks great, @thedtripp. Would it be possible to add:

This is based on commit https://github.com/thedtripp/etcd/commit/4f238837aa1945919b197f300fe7fe244eac4d8c and pull request https://github.com/etcd-io/etcd/pull/14872.

To the backporting commit? Thanks!

etcdserver/verify/doc.go Outdated Show resolved Hide resolved
@thedtripp thedtripp force-pushed the feature/fixFailingReviveChecks branch from bfd3869 to 00bc429 Compare July 3, 2024 03:57
@thedtripp
Copy link
Member Author

thedtripp commented Jul 3, 2024

This looks great, @thedtripp. Would it be possible to add:

This is based on commit https://github.com/thedtripp/etcd/commit/4f238837aa1945919b197f300fe7fe244eac4d8c and pull request https://github.com/etcd-io/etcd/pull/14872.

To the backporting commit? Thanks!

@ivanvc Done. Do you know anything about the failing go vulnerability checker. I'm about to start looking into it out of curiosity. https://github.com/etcd-io/etcd/actions/runs/9771618251/job/26974642695?pr=18215

Edit:

    Denial of service due to improper 100-continue handling in net/http
  More info: https://pkg.go.dev/vuln/GO-2024-2963
  Standard library
    Found in: net/[email protected]
    Fixed in: net/[email protected]
    Example traces found:
Error:       #1: tests/e2e/etcd_process.go:296:28: e2e.BinaryFailpoints.DeactivateHTTP calls http.Client.Do
Error:       #2: etcdserver/cluster_util.go:75:22: etcdserver.getClusterFromRemotePeers calls http.Client.Get
Error:       #3: tools/etcd-dump-metrics/install_linux.go:35:23: etcd.install calls http.Get
Error:       #4: etcdserver/api/rafthttp/transport.go:229:26: rafthttp.Transport.Stop calls http.Transport.CloseIdleConnections
Error:       #5: pkg/transport/transport.go:70:32: transport.unixTransport.RoundTrip calls http.Transport.RoundTrip

Your code is affected by 1 vulnerability from the Go standard library.
This scan found no other vulnerabilities in packages you import or modules you
require.
Use '-show verbose' for more details.
Error: Process completed with exit code 3.```

@ivanvc
Copy link
Member

ivanvc commented Jul 3, 2024

@ivanvc Done. Do you know anything about the failing go vulnerability checker. I'm about to start looking into it out of curiosity. https://github.com/etcd-io/etcd/actions/runs/9771618251/job/26974642695?pr=18215

Please refer to #18269 (which I just opened 😅).

Copy link
Member

@ivanvc ivanvc left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks, @thedtripp.

@thedtripp
Copy link
Member Author

I just noticed I put 2014 as the date in clientv3/internal/resolver/doc.go. I'll fix that and squash.

@thedtripp thedtripp force-pushed the feature/fixFailingReviveChecks branch from 00bc429 to ed27ecb Compare July 3, 2024 04:22
@thedtripp
Copy link
Member Author

/retest

@ivanvc
Copy link
Member

ivanvc commented Jul 3, 2024

@thedtripp, can you rebase this pull request when you get a chance? :)

@thedtripp thedtripp force-pushed the feature/fixFailingReviveChecks branch from ed27ecb to a72fe1b Compare July 3, 2024 23:44
@thedtripp
Copy link
Member Author

/retest

1 similar comment
@thedtripp
Copy link
Member Author

/retest

Copy link
Member

@ivanvc ivanvc left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@jmhbnz jmhbnz left a comment

Choose a reason for hiding this comment

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

LGTM - Thanks @thedtripp

Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

LGTM

Thank you!

Comment on lines +551 to +558
if [ "$GOARCH" == "amd64" ]; then
URL="https://github.com/mgechev/revive/releases/download/${REVIVE_VERSION}/revive_linux_amd64.tar.gz"
elif [[ "$GOARCH" == "arm" || "$GOARCH" == "arm64" ]]; then
URL="https://github.com/mgechev/revive/releases/download/${REVIVE_VERSION}/revive_linux_arm64.tar.gz"
else
echo "Unsupported architecture: $GOARCH"
exit 255
fi
Copy link
Member

Choose a reason for hiding this comment

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

If the local OS is macOS, then contributors need to manually install the tool beforehand.

@ahrtr ahrtr merged commit 518accf into etcd-io:release-3.4 Jul 4, 2024
18 checks passed
@ivanvc ivanvc mentioned this pull request Aug 22, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants