NuGetAuthenticate task #11024
NuGetAuthenticate task #11024
Conversation
zarenner
requested review from
thesattiraju,
hashtagchris,
madhurig and
nishubansal
as code owners
Tasks/MSBuildAuthenticateV0/main.ts
Outdated
| const credProviderAssemblyPath = locateNetFxCredentialProvider(); | ||
| tl.setVariable("NUGET_PLUGIN_PATHS", credProviderAssemblyPath); | ||
| } catch (error) { | ||
| tl.setResult(tl.TaskResult.Failed, error); |
There was a problem hiding this comment.
Shouldn't we send some telemetry back here?
There was a problem hiding this comment.
See my other comment - I'm holding on adding telemetry until we finish deciding how many tasks we'll have (1 vs the current 3).
|
@shubham90 FYI still need to add telemetry. Going to hold on that until we finish deciding if we want multiple tasks still or going back to a single task. |
|
|
||
| tl.debug(`Getting URI for area ID ${areaId} from ${tfsCollectionUrl}`); | ||
| try { | ||
| const serviceUriFromArea = await locationApi.getResourceArea(areaId); |
There was a problem hiding this comment.
When this errors out, it throws a really bad exception message. It would be good to add some additional logging here before throwing it.
There was a problem hiding this comment.
Yeah I just pulled this as-is from the existing utility, seemed odd to me to immediately re-throw like but I assumed there was a historical reason for it. I can definitely add additional logging.
|
Also, I think you'll need to add the new tasks in make-options.json file. Otherwise, build all command won't pick up these tasks. |
zarenner
force-pushed
the
users/zarenner/nugetAuthenticate
branch
2 times, most recently
from
ab7e638 to
4bdb4ac
Compare
Tasks/NuGetAuthenticateV0/main.ts
Outdated
| tl.setResult(tl.TaskResult.Failed, error); | ||
| } finally { | ||
| emitTelemetry("Packaging", "NuGetAuthenticate", { | ||
| "System.TeamFoundationCollectionUri": tl.getVariable("System.TeamFoundationCollectionUri") |
There was a problem hiding this comment.
I'm not emitting anything interesting here yet, and I'm unsure why recording the collection uri is important but we seem to do so in our other tasks.
All the interesting stuff I want to record is in artifacts-common - @jotaylo @shubham90 is there any issue with emitting telemetry multiple times?
There was a problem hiding this comment.
I don't know why we're logging that everywhere, other than potentially making searches by collection a fraction easier. I'm all for removing it.
There's no issue with emitting telemetry multiple times.
zarenner
changed the title
zarenner
force-pushed
the
users/zarenner/nugetAuthenticate
branch
from
4bdb4ac to
03067a5
Compare
zarenner
requested review from
thesattiraju
and removed request for
madhurig,
hashtagchris,
nishubansal and
thesattiraju
zarenner
force-pushed
the
users/zarenner/nugetAuthenticate
branch
from
03067a5 to
9d2a457
Compare
zarenner
force-pushed
the
users/zarenner/nugetAuthenticate
branch
2 times, most recently
from
8fbdad3 to
759a758
Compare
zarenner
force-pushed
the
users/zarenner/nugetAuthenticate
branch
3 times, most recently
from
3b1e8e2 to
8c59576
Compare
zarenner
force-pushed
the
users/zarenner/nugetAuthenticate
branch
from
8c59576 to
b13064a
Compare
| let serviceConnections: ServiceConnection[] = []; | ||
| endpointNames.forEach((endpointName: string) => { | ||
| let uri = tl.getEndpointUrl(endpointName, false); | ||
| let endpointAuth = tl.getEndpointAuthorization(endpointName, true); |
There was a problem hiding this comment.
For some service connections we also have a data parameter, for example Pip and Twine (and now also Maven) we get the endpointname data parameter which we use while generating the authorizations in the respective files.
These parameters come in the form ENDPOINT_DATA_<endpointName>_<key>. Should we add something like an additional properties field to the service connection types and populate them with all the data keys we find? If we want other tasks to also use this library we would have to add some way to enable this functionality or else they will have to write custom logic to just get the data parameters. Let me know your thoughts
There was a problem hiding this comment.
Seems fine, feel free to do this as needed.
| import { IRequestOptions } from 'azure-devops-node-api/interfaces/common/VsoBaseInterfaces'; | ||
| import * as tl from 'azure-pipelines-task-lib/task'; | ||
|
|
||
| export function getWebApiWithProxy(serviceUri: string, accessToken: string): api.WebApi { |
There was a problem hiding this comment.
Should we accept an optional 'options' parameter? For some use cases, for example where we return a redirect to a blobstore url in the content apis, we need to pass in additional options which specify to not forward auth headers (like the options defined here: https://github.com/microsoft/typed-rest-client/blob/master/lib/HttpClient.ts#L392). We can merge the users and our options before creating the client.
There was a problem hiding this comment.
Fine with me, feel free to change it as needed when you merge your changes with this.
zarenner
force-pushed
the
users/zarenner/nugetAuthenticate
branch
from
b13064a to
aa12108
Compare
No description provided.