Skip to content

Conversation

@danopia
Copy link

@danopia danopia commented May 14, 2019

Hey, I have a situation where I want to include a nonstandard HTTP header on Jenkins requests which authorize them to get past a firewalling proxy.

I didn't see a way to attach custom headers using this library so I added an extension point in this PR to accept arbitrary key/value pairs at the JenkinsClient level.

I'm not sure if there's a decent way to test this, as it seems the tests assume a localhost Jenkins, and Rider isn't hooking up to xUnit for me to begin with. Perhaps the best way to test this change would be manually constructing and passing an Authorization header instead of passing Username and ApiToken (so, Authorization: Basic Z3Vlc3Q6YmNiOTU0YTc3YWI0Nzc1MDIwMWE5ZjE4OGIxYjI1ZTg=)
Indeed, this PR offers a superset of the Username/ApiToken functionality.


A possible different approach could be allowing a global OnWrite callback. Even more flexible but also easier to go wrong

@danopia danopia changed the title Add 'ExtraHeaders' dictionary to all Commands Introduce 'ExtraHeaders' capability across all Commands May 14, 2019
@mavenjones
Copy link

nice

Rider suggested this, but it breaks in netstandard2.0
@null511
Copy link
Owner

null511 commented May 28, 2019

Since this project is already using Basic Authentication through HTTP using the JenkinsClient.Username/Password, I'm not sure exactly what change your after; you should be able to use the existing Username/Password properties to create a basic authentication header. Are you using an alternate header name than Authorization?

This looks good though, but I'm not sure if it's the right approach. Instead I would prefer to add a "BeforeRequest" event that could be used to modify the HttpRequest object directly; this should be a simpler implementation that allows much more control.

@null511
Copy link
Owner

null511 commented May 28, 2019

I've implemented the above suggestion in #24 - please let me know if this meets your minimum requirements.

@danopia
Copy link
Author

danopia commented Jun 18, 2019

Hey, sorry for the delay, just got around to testing this on my side again. Turns out my PR is incomplete so I've switched to the implementation you generously implemented into the 1.0.5 release branch.

I had to make a small patch for full coverage, as RunAsync didn't get your OnBeforeRequestSend call added:

diff --git a/Jenkins.Net/Internal/JenkinsHttpCommand.cs b/Jenkins.Net/Internal/JenkinsHttpCommand.cs
index fc62312..9719e54 100644
--- a/Jenkins.Net/Internal/JenkinsHttpCommand.cs
+++ b/Jenkins.Net/Internal/JenkinsHttpCommand.cs
@@ -52,6 +52,8 @@ namespace JenkinsNET.Internal
 
             if (OnWriteAsync != null) await OnWriteAsync.Invoke(request, token);
 
+            client.OnBeforeRequestSend(request);
+
             using (token.Register(() => request.Abort(), false))
             using (var response = (HttpWebResponse)await request.GetResponseAsync()) {
                 token.ThrowIfCancellationRequested();

Other than that, #24 looks good from here! I'll go ahead and close this PR.

@danopia danopia closed this Jun 18, 2019
@null511
Copy link
Owner

null511 commented Jun 18, 2019

Oh man, nice catch. Thanks for getting that together and following up, I'll get another release out asap.

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.

3 participants