-
Notifications
You must be signed in to change notification settings - Fork 2
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
Added isSecure field in the winrm settings. When isSecure is true, we allow HTTPs connection to end point powershell. If isSecure is false, we allow HTTP connection to end point powershell. #5
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see there isn't a ton of tests in this repo... but how about adding one for your change?
winrm.go
Outdated
@@ -27,6 +27,7 @@ const ( | |||
NTLM | |||
Kerberos | |||
) | |||
const WINRM_HTTP_PORT = 5985 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a detail but this is unused... so I'm wondering why you're adding it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed
winrm.go
Outdated
@@ -55,6 +56,8 @@ type winrmSettings struct { | |||
operationTimeout string | |||
// Timeout of each HTTP call made | |||
timeout int | |||
//Whether the call is http or https |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nits:
- Missing space before "Whether"
- http -> HTTP / https -> HTTPS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed
@@ -185,7 +196,11 @@ func NewWinRMClient(details getEndpointDetails, options ...winrmSettingsOption) | |||
for _, o := range options { | |||
client.winrmSettings = o(client.winrmSettings) | |||
} | |||
client.url = fmt.Sprintf("https://%s:%d/wsman", client.ipAddress, client.port) | |||
if client.isSecure { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a detail but to avoid repeating the whole Sprintf
line, how about:
if client.isSecure {
protocol = "https"
} else {
protocol = "http"
}
then use protocol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed
@@ -27,6 +27,10 @@ const ( | |||
NTLM | |||
Kerberos | |||
) | |||
const ( | |||
HTTP_PROTOCOL = "http" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a detail but http/https is usually referred as the "scheme".
@@ -65,3 +65,21 @@ func TestCaptureAttribute(t *testing.T) { | |||
t.Error("invalid attribute") | |||
} | |||
} | |||
|
|||
func BenchmarkHTTPRequest(b *testing.B) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how this is relevant...?
No description provided.