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

feature: support basic authentication when using a proxy #1069

Merged
merged 7 commits into from
May 30, 2023
Merged

feature: support basic authentication when using a proxy #1069

merged 7 commits into from
May 30, 2023
+455 −329

Conversation

drazisil-codecov
Copy link
Contributor

closes #1067

@drazisil-codecov drazisil-codecov linked an issue May 26, 2023 that may be closed by this pull request
Feature Request: Support basic authentication when using a proxy #1067
Closed
@drazisil-codecov
refactor: rename to match args format

Verified

This commit was signed with the committer’s verified signature.
drazisil-codecov Joe Becher
GPG key ID: 42BBF73B175480C4
Verified
Learn about vigilant mode
a28f7a1 
Signed-off-by: Joe Becher <joe.becher@sentry.io>
@drazisil-codecov drazisil-codecov force-pushed the 1067-feature-request-support-basic-authentication-when-using-a-proxy branch from c784dae to a28f7a1 Compare May 26, 2023 14:25
@drazisil-codecov drazisil-codecov marked this pull request as ready for review May 26, 2023 17:32
@drazisil-codecov drazisil-codecov requested a review from a team as a code owner May 26, 2023 17:33
@codecov
Copy link

codecov bot commented May 26, 2023 •
edited
Loading

Codecov Report

Merging #1069 (759214d) into main (3be8503) will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1069      +/-   ##
==========================================
+ Coverage   92.34%   92.42%   +0.08%     
==========================================
  Files          35       36       +1     
  Lines        1319     1333      +14     
  Branches      270      272       +2     
==========================================
+ Hits         1218     1232      +14     
  Misses         69       69              
  Partials       32       32
Flag Coverage Δ
aarch64 92.34% <100.00%> (+0.08%) ⬆️
aarch64-without-git 92.34% <100.00%> (+0.08%) ⬆️
alpine 92.34% <100.00%> (+0.08%) ⬆️
alpine-proxy 92.34% <100.00%> (+0.08%) ⬆️
alpine-without-git 92.34% <100.00%> (+0.08%) ⬆️
linux 92.34% <100.00%> (+0.08%) ⬆️
linux-without-git 92.34% <100.00%> (+0.08%) ⬆️
macos 92.34% <100.00%> (+0.08%) ⬆️
macos-without-git 92.34% <100.00%> (+0.08%) ⬆️
windows 91.74% <100.00%> (+0.08%) ⬆️
windows-without-git 91.74% <100.00%> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/ci_providers/provider_appveyorci.ts 94.28% <100.00%> (ø)
src/ci_providers/provider_azurepipelines.ts 98.11% <100.00%> (ø)
src/ci_providers/provider_bitbucket.ts 100.00% <100.00%> (ø)
src/ci_providers/provider_bitrise.ts 100.00% <100.00%> (ø)
src/ci_providers/provider_buildkite.ts 93.75% <100.00%> (ø)
src/ci_providers/provider_circleci.ts 100.00% <100.00%> (ø)
src/ci_providers/provider_cirrus.ts 96.66% <100.00%> (ø)
src/ci_providers/provider_codebuild.ts 96.66% <100.00%> (ø)
src/ci_providers/provider_drone.ts 93.54% <100.00%> (ø)
src/ci_providers/provider_githubactions.ts 95.89% <100.00%> (ø)
... and 13 more

thomasrockhu-codecov
thomasrockhu-codecov requested changes May 30, 2023
View reviewed changes
Copy link
Contributor

@thomasrockhu-codecov thomasrockhu-codecov left a comment

Choose a reason for hiding this comment

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

Thanks for all the cleanup changes! I know I'm guilty of it, but for future PRs, maybe put them in a separate PR. Makes it a lot easier to review.

src/helpers/proxy.ts
import { ProxyAgent } from 'undici';
import { UploaderArgs, UploaderEnvs } from '../types';

export function addProxyIfNeeded(envs: UploaderEnvs, args: UploaderArgs): ProxyAgent | undefined {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this needs to be in it's own file. It's only being used in web

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it was easier to locate it if it needed to be changed, but I can put it back in web if you would like.

Copy link
Contributor

Choose a reason for hiding this comment

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

we could put the generateHeaders function here then if we want proxy stuff to be in one file.

src/helpers/web.ts Outdated
method: 'POST',
origin: postURL,
path: `${url.pathname}${url.search}`,
},
}
}

export function generateHeaders(envs: UploaderEnvs, initialHeaders: IncomingHttpHeaders): IncomingHttpHeaders {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not convinced this needs to be it's own function. It feels particularly dirty since the function is called generateHeaders, but we're giving it headers already. I think we should deal with all header generation here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't want a code branch here, since POST and PUT use different headers.

Copy link
Contributor

Choose a reason for hiding this comment

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

could we just name it something like addProxyHeaders then?

@drazisil-codecov
Copy link
Contributor Author

Thanks for all the cleanup changes! I know I'm guilty of it, but for future PRs, maybe put them in a separate PR. Makes it a lot easier to review.

Yah. The refactor was needed for this PR, which is why I did it. I couldn't easily get the proxy creds to the web parts otherwise.

thomasrockhu-codecov
thomasrockhu-codecov approved these changes May 30, 2023
View reviewed changes
Copy link
Contributor

@thomasrockhu-codecov thomasrockhu-codecov left a comment

Choose a reason for hiding this comment

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

LGTM barring CI

@drazisil-codecov drazisil-codecov merged commit 2ad9d12 into main May 30, 2023
@drazisil-codecov drazisil-codecov deleted the 1067-feature-request-support-basic-authentication-when-using-a-proxy branch May 30, 2023 15:36
This was referenced May 30, 2023
Merged
Merged
drazisil-codecov added a commit that referenced this pull request May 30, 2023
@drazisil-codecov
feature: support basic authentication when using a proxy (#1069)
7f2fd4d 
* feat: add basic auth to proxy call

---------

Signed-off-by: Joe Becher <joe.becher@sentry.io>
@SUNx2YCH SUNx2YCH mentioned this pull request Jun 19, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomasrockhu-codecov thomasrockhu-codecov

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

Successfully merging this pull request may close these issues.

Feature Request: Support basic authentication when using a proxy
2 participants
@drazisil-codecov @thomasrockhu-codecov