Pip and Twine Auth tasks V1 #11026
Pip and Twine Auth tasks V1 #11026
Conversation
| } | ||
|
|
||
| export async function getInternalAuthInfoArray(inputKey: string): Promise<AuthInfo[]> { | ||
| let internalAuthArray: AuthInfo[] = []; |
There was a problem hiding this comment.
This code should also go away after merging with artifacts-common
There was a problem hiding this comment.
waiting for merge from Aasim's commit
Tasks/PipAuthenticateV1/make.json
Outdated
| { | ||
| "common": [ | ||
| { | ||
| "module": "../Common/utility-common", |
There was a problem hiding this comment.
can you switch to using utility-common-v2 instead (which uses azure-pipelines-task-lib instead of vsts-task-lib)
Tasks/TwineAuthenticateV1/make.json
Outdated
| { | ||
| "common": [ | ||
| { | ||
| "module": "../Common/utility-common", |
There was a problem hiding this comment.
utility-common-v2 here too
There was a problem hiding this comment.
@zarenner and I were talking about this yesterday. The only thing we use utility-common is for telemetry. Zach was going to look for options to remove this dependency altogether. However, he did say that utility-common-v2 has some very specific code from some other team, so not sure if it's worth taking dependency on.
There was a problem hiding this comment.
Yeah, I just see that @zarenner is adding it to the new shared lib, that'll do the trick.
Tasks/PipAuthenticateV1/Strings/resources.resjson/en-US/resources.resjson
Outdated
Show resolved
Hide resolved
|
|
||
| telemetry.emitTelemetry("Packaging", "PipAuthenticate", pipAuthenticateTelemetry); | ||
| } catch (err) { | ||
| tl.debug(`Unable to log Pip Authenticate task init telemetry. Err:( ${err} )`); |
There was a problem hiding this comment.
I think this is effectively unreachable - emitTelemetry has its own try/catch.
| @@ -0,0 +1,21 @@ | |||
| import * as tl from 'azure-pipelines-task-lib/task'; | |||
|
|
|||
| export function getProjectAndFeedIdFromInputParam(inputParam: string): any { | |||
There was a problem hiding this comment.
not used currently, but kept it anyway when copying from packaging-common.
There was a problem hiding this comment.
I'd remove it, personally. It doesn't do much, and IMO it's more clear when inputs are read from within the task itself when reasonable to do so.
| return getProjectAndFeedIdFromInput(feedProject); | ||
| } | ||
|
|
||
| export function getProjectAndFeedIdFromInput(feedProject: string): any { |
There was a problem hiding this comment.
Create an interface for { feedId:string, projectId: string}? e.g. ProjectScopedFeed
And possibly rename function based on that (or at least remove "FromInput" which doesn't really convey anything)?
| * @param feedId FeedId to put in api params | ||
| * @param project projectId to put in api params | ||
| */ | ||
| export async function getPackagingEndpointUrl( |
There was a problem hiding this comment.
Suggest rename to "getPackagingRouteUrl" or something like that.
Also, this function assumes that the only route parameters that need to be filled are feedId and project, so if a route has other inputs this will return a route template rather than a route I think? Should make that clear and possibly allow specifying additional route values.
There was a problem hiding this comment.
cool, I thought we won't need any other params in auth tasks. But, I can add an optional dictionary here.
There was a problem hiding this comment.
A short comment explaining the above would be fine instead of implementing it now.
e.g. this function wouldn't work for routes that have {packageName}, {packageVersion}, etc if we ever needed to get one of those.
| feedId: string, | ||
| project: string): Promise<string> { | ||
|
|
||
| tl.debug('Finding the URI for the packaging service'); |
| const webApi = api.getWebApiWithProxy(serviceUri, accessToken); | ||
| const data = await webApi.vsoClient.getVersioningData(apiVersion, protocols.ProtocolType[protocolType], locationGuid, { feedId: feedId, project: project }); | ||
|
|
||
| tl.debug('Feed registry url: ' + data.requestUrl); |
There was a problem hiding this comment.
"Feed registry url" doesn't really accurately describe what this returns. Perhaps something like "Resolved route: "
No description provided.