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.
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.
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.
Do we really need to signalize about it in stdout (not even on debug level)?
| @@ -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.
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.
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() |
kirill-ivlev
force-pushed
the
users/kirill-ivlev/add-proxy-formatted-url
branch
from
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 full proxy URL (http://user:password@host) for the getHttpProxyMethod
And cover this method with the unit tests.