-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Docker task using spn creds for authentication in ACR #4053
Conversation
"q": "1.4.1", | ||
"vso-node-api": "6.0.1-preview", | ||
"vsts-task-lib": "2.0.1-preview", | ||
"xml2js": "0.4.13" |
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.
remove it.
public getAuthenticationToken(): RegistryAuthenticationToken | ||
{ | ||
if(this.registryURL && this.endpointName) { | ||
|
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.
remove it
interface MochaSetupOptions { | ||
//milliseconds to wait before considering a test slow | ||
slow?: number; | ||
|
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.
Better to move it out.
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.
As Subhu explained each module has there own typings.
private endpointName: string; | ||
|
||
// ACR fragment like /subscriptions/c00d16c7-6c1f-4c03-9be1-6934a4c49682/resourcegroups/jitekuma-RG/providers/Microsoft.ContainerRegistry/registries/jitekuma | ||
private acrFragmentUrl: 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.
do you still need these portions of the code?
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.
We need registryURL, endpointName but acrFragmentUrl is extra but i kept it as this is unique url for the registry in ACR and i want to keep it, in case we require in future to make rest call. Currently we are not using acrFragmentUrl.
@@ -21,38 +21,28 @@ else { | |||
authenticationProvider = new GenericAuthenticationTokenProvider(tl.getInput("dockerRegistryEndpoint")); | |||
} | |||
|
|||
authenticationProvider.getAuthenticationToken().then( | |||
function success(registryAuthenticationToken) { |
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.
why remove this success method?
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.
Now Authentication Provider is not returning promise its returning authentication token directly, since rest call is not required anymore. sucess and failure method only exists for the promise.
@@ -25,52 +25,41 @@ else { | |||
authenticationProvider = new GenericAuthenticationTokenProvider(tl.getInput("dockerRegistryEndpoint")); | |||
} | |||
|
|||
authenticationProvider.getAuthenticationToken().then( | |||
function success(registryAuthenticationToken) { |
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.
Same as above
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.
Now Authentication Provider is not returning promise its returning authentication token directly, since rest call is not required anymore. sucess and failure method only exists for the promise.
Docker task using spn creds for authentication in ACR