-
Notifications
You must be signed in to change notification settings - Fork 274
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 formatted proxy URL #834
added formatted proxy URL #834
Conversation
@mmrazik Could you please review this PR |
@@ -1767,6 +1767,7 @@ export function findMatch(defaultRoot: string, patterns: string[] | string, find | |||
|
|||
export interface ProxyConfiguration { | |||
proxyUrl: string; | |||
proxyFormattedUrl: string; |
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.
What does proxyFormattedUrl mean - what is the format/purpose/how to use it? Could it make sense to add JSDoc comments here?
Please update docs as well.
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.
JSDoc added
Docs updated
node/task.ts
Outdated
if (proxyUsername) { | ||
proxyAddress = `${parsedUrl.protocol}//${proxyUsername}:${proxyPassword}@${parsedUrl.host}`; | ||
} | ||
console.log('returning proxy config'); |
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.
Do we really need to signalize about it in stdout (not even on debug level)?
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.
removed
@@ -1767,6 +1767,7 @@ export function findMatch(defaultRoot: string, patterns: string[] | string, find | |||
|
|||
export interface ProxyConfiguration { | |||
proxyUrl: string; | |||
proxyFormattedUrl: string; |
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 field is not orthogonal to the other fields, so it could be error-prone in future (it depends on values of the fields like proxyUrl/proxyUsername, so if they will be changed - we would need to change it as well).
Could it make sense to just add some function which return formatted url - instead of adding new field to avoid such issue?
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.
Resolved per offline discussion - since we considering that this is intended to be read-only object (containing current proxy settings) - it should be good to keep in a separate field
Could you please bump package version as well (patch number)? |
LGTM, thanks! Please take a look at the comments also |
import tl = require('azure-pipelines-task-lib/task'); | ||
|
||
async function run() { | ||
let proxy = tl.getProxyConfiguration() |
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.
nit: const
fd046dd
to
458f4a1
Compare
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
@mmrazik would you mind taking a look at this pull request? |
* added formatted proxy URL * version increment
Added full proxy URL (http://user:password@host) for the getHttpProxyMethod
And cover this method with the unit tests.