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

Expose VaultSharp's PostProcessHttpClientHandlerAction hook to allow configuring proxy settings, etc. #52

Conversation

deberhar
Copy link
Contributor

Hi @MrZoidberg, here's a draft PR implementing approach 1 as discussed on #47.

I did test the main project change, but I unfortunately don't have Docker for Windows available (my org doesn't license it); the added unit test is blind-coded. Would you be able to run the test and verify it passes prior to merging, please?

@MrZoidberg
Copy link
Owner

@deberhar looks fine. There is a problem with your tests https://github.com/MrZoidberg/VaultSharp.Extensions.Configuration/actions/runs/7730287609/job/21283577612?pr=52 - should be easy to fix.

@MrZoidberg MrZoidberg self-requested a review February 6, 2024 17:23
@deberhar
Copy link
Contributor Author

deberhar commented Feb 6, 2024

I see... looks like _logger was renamed to logger in 340996f.

Let me rebase this PR to pull in that change. One moment...

Expose PostProcessHttpClientHandlerAction via the VaultSharp configuration
provider settings.  This allows arbitrary customizations to the underlying
HTTP client, such as customizing HTTP proxy settings.
@deberhar deberhar force-pushed the feature/PostProcessHttpClientHandlerAction branch from 6dc1395 to 0992b65 Compare February 6, 2024 17:46
@deberhar
Copy link
Contributor Author

deberhar commented Feb 6, 2024

Okay... if I did that right, the PR branch is now rebased onto master, including the one commit to add the new property + test, reparented onto 340996f and reflecting the logger field rename.

@deberhar
Copy link
Contributor Author

deberhar commented Feb 6, 2024

Looks like the build completed and the test ran okay:

Passed!  - Failed:     0, Passed:    13, Skipped:     0, Total:    13, Duration: 1 m 10 s - VaultSharp.Extensions.Configuration.Test.dll (net8.0)

...but the build wasn't able to add a code-coverage comment on this PR. I think it's this issue: marocchino/sticky-pull-request-comment#930 (comment). Per the linked article:

[...] a new trigger workflow_run was introduced to enable scenarios that require building the untrusted code and also need write permissions to update the PR with e.g. code coverage results or other test results. To do this in a secure manner, the untrusted code must be handled via the pull_request trigger so that it is isolated in an unprivileged environment. The workflow processing the PR should then store any results like code coverage or failed/passed tests in artifacts and exit. The following workflow then starts on workflow_run where it is granted write permission to the target repository and access to repository secrets, so that it can download the artifacts and make any necessary modifications to the repository or interact with third party services that require repository secrets (e.g. API tokens).

Do you want me to apply that Cancel() -> CancelAsync() suggestion before you merge?

@MrZoidberg
Copy link
Owner

that's fine since it's a test

@MrZoidberg
Copy link
Owner

Good to merge. thanks for your contribution. I'll try to release a fix today.

@MrZoidberg MrZoidberg merged commit 7037831 into MrZoidberg:master Feb 6, 2024
1 check failed
@deberhar
Copy link
Contributor Author

deberhar commented Feb 6, 2024

Thank you, much appreciated!

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