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

DEVPROD-7742 Fix current bodyclose lint issues #8601

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ZackarySantana
Copy link
Contributor

DEVPROD-7742

Description

Before merging in the fix for our linter and adding stricter rules, I'm doing smaller PRs to fix the current mistakes that our linter should be catching.

This PR adds the bodyclose linter and fixes our current cases where we do not close the body. It does have some false positives with helper functions in particular (which usually it's good about but I had to add some nolint directives for them).

I also fixed it for the tests just to be semantic, but it probably isn't that important for them.

Testing

Running golangci-lint run in my terminal results in no more lint errors. This pulls from the .golangci.yml file's configuration (which our linter task should be doing.

@ZackarySantana ZackarySantana requested a review from a team January 8, 2025 19:42
@ZackarySantana ZackarySantana self-assigned this Jan 8, 2025
@ZackarySantana
Copy link
Contributor Author

evergreen retry

@@ -492,6 +492,7 @@ func (c *baseCommunicator) GetCedarConfig(ctx context.Context) (*apimodels.Cedar
if err != nil {
return nil, util.RespErrorf(resp, errors.Wrap(err, "getting the Cedar config").Error())
}
defer resp.Body.Close()
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't necessary because utility.ReadJSON closes the body already (same with the others in this function). Since most of the methods share the same logic (make request, read the JSON body) is there a reason some of the methods have been changed to defer close the body and others have been changed to nolint?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants