-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
[3.4] installed revive on CI and added doc.go to fix revive linter errors #18215
Conversation
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 Once the patch is verified, the new status will be reflected by the 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. |
What are your thoughts on this PR @jmhbnz? |
This looks good overall. I wonder if a better approach would be to add a |
/ok-to-test |
/retitle [3.4] formatting: added package comments to fix revive linter errors. |
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 |
Please ensure the linter isn't skipped firstly, then resolve the errors detected by the linter.
OK for me. Actually most of the packages already have |
@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). |
4f15506
to
83c1c4c
Compare
29d05f7
to
731d92c
Compare
731d92c
to
29d05f7
Compare
7e4a399
to
b0d4ea2
Compare
/retitle [3.4] installed revive on CI and added doc.go to fix revive linter errors. |
@thedtripp: Re-titling can only be requested by trusted users, like repository collaborators. In response to this:
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. |
/retitle [3.4] installed revive on CI and added doc.go to fix revive linter errors |
b0d4ea2
to
692822d
Compare
/joke |
@thedtripp: How can you tell a vampire has a cold? They start coffin. In response to this:
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. |
There was a problem hiding this 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!
bfd3869
to
00bc429
Compare
@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:
|
Please refer to #18269 (which I just opened 😅). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks, @thedtripp.
I just noticed I put 2014 as the date in clientv3/internal/resolver/doc.go. I'll fix that and squash. |
00bc429
to
ed27ecb
Compare
/retest |
@thedtripp, can you rebase this pull request when you get a chance? :) |
This is based on commit 4f23883 and pull request etcd-io#14872. Signed-off-by: D Tripp <38776199+thedtripp@users.noreply.github.com>
Signed-off-by: D Tripp <38776199+thedtripp@users.noreply.github.com>
ed27ecb
to
a72fe1b
Compare
/retest |
1 similar comment
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - Thanks @thedtripp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thank you!
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 |
There was a problem hiding this comment.
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.
This is meant to address the failing revive checks mentioned in this issue: #17472
Screenshot of Revive linter errors:
Output with code changes: