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

Brushes off some lint #452

Merged
merged 1 commit into from
Oct 9, 2023
Merged

Brushes off some lint #452

merged 1 commit into from
Oct 9, 2023

Conversation

ChuckCrawford
Copy link
Contributor

@ChuckCrawford ChuckCrawford commented Oct 3, 2023 •
edited
Loading

Overview

  • Resolves most go staticcheck errors
  • Replaces a few deprecated packages / functions.
  • Makes some minor changes to error responses on the /json API endpoint (see the tests in test/server/server_impl_test.go for more detail).

Commentary

  • Left ST1006 alone for now since this is used all over the place.
  • Left the server.runtime unused import alone since I was not 100% sure whether it was safe to remove.
  • The minor changes to the error responses on the /json API endpoint were motivated by:
    • The deprecation of github.com/golang/protobuf/jsonpb which is used to convert protobuf to JSON. Some of the error messages were coming from the internals of this library and were no longer available once replaced with google.golang.org/protobuf/encoding/protojson.
    • It is generally a good security practice not to leak library internals back out to clients. [Though I am sure it is rare to expose something like this externally.]

@ChuckCrawford
Lint Cleanup
0904ca1 
* Resolves most go staticcheck errors.
* Replaces a few deprecated packages / functions.
* Makes some minor changes to error responses on the /json endpoint.

Signed-off-by: Charlie Crawford <ccrawford@pagerduty.com>
@ChuckCrawford ChuckCrawford changed the title Brushing off some lint Brushes off some lint Oct 3, 2023
@ChuckCrawford
Copy link
Contributor Author

@envoyproxy/ratelimit-maintainers in the event you are just really bored and looking for something to do 😉

@ysawa0
Copy link
Member

ysawa0 commented Oct 6, 2023

Very nice. Just to confirm so these lints were all detected via tooling and safe to update, correct?

@ChuckCrawford
Copy link
Contributor Author

Very nice. Just to confirm so these lints were all detected via tooling and safe to update, correct?

That is correct. Everything I changed was detected by go staticcheck. The only functional change here is very minor changes to the error responses of the /json API since the previous error responses were leaking library-specific details [and that library was deprecated].

ysawa0
ysawa0 reviewed Oct 7, 2023
View reviewed changes
src/server/server_impl.go
logger.Warnf("error: %s", err.Error())
http.Error(writer, err.Error(), http.StatusBadRequest)
Copy link
Member

Choose a reason for hiding this comment

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

Any particular reason we're removing the more descriptive error msgs in the responses? Is it for consistency, security?

Copy link
Contributor Author

@ChuckCrawford ChuckCrawford Oct 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ah my apologies! I have updated the PR description to provide more clarity around why I made these overall changes here.

In this exact line you commented on we are still doing the same thing; I just refactored it into a shared function for consistency.

@ysawa0 ysawa0 merged commit b979623 into envoyproxy:main Oct 9, 2023
timcovar pushed a commit to goatapp/ratelimit that referenced this pull request Jan 16, 2024
@ChuckCrawford @timcovar
Lint Cleanup (envoyproxy#452)
5085bb8 
* Resolves most go staticcheck errors.
* Replaces a few deprecated packages / functions.
* Makes some minor changes to error responses on the /json endpoint.

Signed-off-by: Charlie Crawford <ccrawford@pagerduty.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ysawa0 ysawa0 ysawa0 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ChuckCrawford @ysawa0