-
Notifications
You must be signed in to change notification settings - Fork 1.3k
all for both self certs and password at the same time #6274
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
all for both self certs and password at the same time #6274
Conversation
} else if (value === closeOption) { | ||
sendTelemetryEvent(Telemetry.SelfCertsMessageClose); | ||
} | ||
// Don't leave our Interactive Window open in a non-connected state |
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 can remove this if we don't like it. I still have the bug for doing a reconnect that I'll try to fit into this release. But I felt this one liner made the scenario better than it currently is. So I stuck it in with this fix.
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.
No sounds good to me. Better than what we have now.
In reply to: 296046769 [](ancestors = 296046769)
} | ||
} | ||
|
||
// For HTTPS connections respect our allowUnauthorized setting by adding in an agent to enable that on the request |
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.
Our services connection stuff was using the allowUnauthorized flag, but the manual password requests were not. So you couldn't connect to a self cert password machine.
let xsrfCookie: string | undefined; | ||
|
||
const response = await fetchFunction(`${url}login?`, { | ||
const response = await fetchFunction(`${url}login?`, this.allowUnauthorized(url, allowUnauthorized, { |
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.
allowUnauthorized [](start = 66, length = 17)
Naming here is weird. Parameter and function have the same name?. Maybe it could be called getAllowUnauthorized or something?
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.
The parameter is to check if its allowed right? Why not name it 'unauthorizedIsAllowed'? That seems to describe the parameter to me and the function name could stay like that.
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.
Yeah, that kinda slipped by while coding. It's not good. I'll rename
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 went with renaming the function as the allowUnauthorized reflects a setting / bool to me, but not an action. So renamed the function to better express the action being performed.
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.
let xsrfCookie: string | undefined; | ||
|
||
const response = await fetchFunction(`${url}login?`, { | ||
const response = await fetchFunction(`${url}login?`, this.allowUnauthorized(url, allowUnauthorized, { |
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.
The parameter is to check if its allowed right? Why not name it 'unauthorizedIsAllowed'? That seems to describe the parameter to me and the function name could stay like that.
For #6265
package-lock.json
has been regenerated by runningnpm install
(if dependencies have changed)