Skip to content

feat: switch to using actions/http-client#94

Merged
bharathkkb merged 7 commits intomainfrom
fix-proxy
Feb 14, 2022
Merged

feat: switch to using actions/http-client#94
bharathkkb merged 7 commits intomainfrom
fix-proxy

Conversation

@bharathkkb
Copy link
Copy Markdown
Contributor

@bharathkkb bharathkkb commented Feb 11, 2022

switch to making requests via actions/http-client which respects proxy env vars

@bharathkkb bharathkkb requested a review from a team as a code owner February 11, 2022 19:46
@bharathkkb bharathkkb marked this pull request as draft February 11, 2022 19:46
@bharathkkb bharathkkb marked this pull request as ready for review February 11, 2022 22:44
throw new Error(`Failed to retrieve gcloud SDK version, invalid response body: ${body}`);
export async function getGcloudVersion(url: string): Promise<string> {
const http = new HttpClient(userAgentString, undefined, { allowRetries: true, maxRetries: 3 });
const res = await http.getJson<{ version: string }>(url);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It doesn't look like the library actually checks 400s or 500s. I think we need to get the body first, then check the response, then print the body on error. Otherwise we risk swallowing the message.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The getJson does check here but since we want op I will switch to the other approach.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh that's a weird API. The README specifically states that errors are not checked. And it looks like that's true for non-JSON responses.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

getJson could be useful for us if it threw an error for non-JSON responses. I will open an issue but doubt it will happen since its a breaking change for them.

@bharathkkb bharathkkb requested a review from sethvargo February 12, 2022 00:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants