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

Ensure response body is always closed in CloudControllerConnection #2423

Merged
merged 1 commit into from
Jan 18, 2024
Merged

Ensure response body is always closed in CloudControllerConnection #2423

merged 1 commit into from
Jan 18, 2024

Conversation

chitoku-k
Copy link
Contributor

@chitoku-k chitoku-k commented May 18, 2023 •
edited
Loading

Does this PR modify CLI v6, CLI v7, or CLI v8?

main

Description of the Change

This PR ensures that a response body returned from HTTPClient in CloudControllerConnection.Make() is correctly closed in all of the following circumstances:

  • when the status code of the response is http.StatusNoContent
  • when the handling of X-Cf-Warnings returns an error

Why Is This PR Valuable?

Some projects import cloudfoundry/cli as a means to interact with CAPI and they should benefit from this fix.

Why Should This Be In Core?

N/A

Applicable Issues

N/A

How Urgent Is The Change?

There should be a few projects that are currently affected by this bug such as bosh-prometheus/cf_exporter.

Other Relevant Parties

None.

a-b
a-b approved these changes May 18, 2023
View reviewed changes
Copy link
Member

@a-b a-b left a comment

Choose a reason for hiding this comment

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

lgtm

@a-b
Copy link
Member

a-b commented May 18, 2023

Please create PRs for:

This was referenced May 18, 2023
Merged
Merged
@chitoku-k
Copy link
Contributor Author

Thank you for the prompt response. I've created #2424 and #2425, for v7 and v8 branch respectively.

@a-b a-b closed this Jun 7, 2023
@a-b a-b reopened this Jun 7, 2023
@a-b a-b force-pushed the fix/close-response-body branch 2 times, most recently from 955db0c to 17a4ae8 Compare June 8, 2023 04:13
@a-b a-b force-pushed the fix/close-response-body branch 2 times, most recently from fab3836 to b8e2f8d Compare June 15, 2023 14:08
@a-b a-b force-pushed the fix/close-response-body branch from b8e2f8d to 8415e6b Compare July 13, 2023 21:03
a-b
a-b requested changes Jan 9, 2024
View reviewed changes
Copy link
Member

@a-b a-b left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Per additional review with @moleske , @ccjaimes we recommending moving defer even closer to where we opening connection here

@chitoku-k chitoku-k force-pushed the fix/close-response-body branch from 8415e6b to a1b1bfb Compare January 10, 2024 02:26
@chitoku-k
Copy link
Contributor Author

Thanks, I have modified the commit in this and the other two PRs that way.

@ccjaimes
Copy link
Contributor

ccjaimes commented Jan 11, 2024 •
edited
Loading

@chitoku-k Thanks for your fast response on these changes.

@ccjaimes ccjaimes requested a review from a-b January 11, 2024 19:52
@moleske moleske merged commit 431f4e0 into cloudfoundry:main Jan 18, 2024
13 checks passed
@chitoku-k chitoku-k deleted the fix/close-response-body branch January 19, 2024 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ccjaimes ccjaimes ccjaimes approved these changes

@a-b a-b a-b 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.
5 participants
@chitoku-k @a-b @ccjaimes @cf-gitbot @moleske