Decouple NpmAuthenticateV0 from packaging-common/npm modules#21911
Decouple NpmAuthenticateV0 from packaging-common/npm modules#21911
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
…github.com/microsoft/azure-pipelines-tasks into users/alextorres/NpmAuthenticateV0Refactor
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR decouples NpmAuthenticateV0 from the azure-pipelines-tasks-packaging-common/npm/* modules by inlining the required npmrc parsing/normalization, credential formatting, and backup/restore logic into the task itself, while keeping locationUtilities as the remaining shared dependency.
Changes:
- Introduces local implementations for
.npmrcregistry discovery/mutation, endpoint credential resolution, and backup/restore (newnpmauthutils,npmrcCredential,npmrcBackupManager). - Updates task runtime and post-job cleanup to use the new local utilities instead of packaging-common npm helpers.
- Reworks and expands unit/L0 tests to exercise the real code paths (including new endpoint credential and error-path suites), and bumps task versions/strings accordingly.
Reviewed changes
Copilot reviewed 77 out of 81 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| _generated/NpmAuthenticateV0_Wif/task.loc.json | Version bump + adds InvalidRegistryUrl message mapping for WIF build. |
| _generated/NpmAuthenticateV0_Wif/task.json | Version bump + adds InvalidRegistryUrl message for WIF build. |
| _generated/NpmAuthenticateV0_Wif/package.json | Adds ini dependency for local npmrc parsing. |
| _generated/NpmAuthenticateV0_Wif/package-lock.json | Locks ini dependency for WIF build output. |
| _generated/NpmAuthenticateV0_Wif/npmrcCredential.ts | Adds local endpoint credential resolution + internal/external probing. |
| _generated/NpmAuthenticateV0_Wif/npmrcBackupManager.ts | Adds local backup/restore manager for .npmrc files. |
| _generated/NpmAuthenticateV0_Wif/npmauthutils.ts | Adds local helpers for registry normalization, parsing, auth appends, and cleanup. |
| _generated/NpmAuthenticateV0_Wif/npmauthcleanup.ts | Switches cleanup from packaging-common util to NpmrcBackupManager. |
| _generated/NpmAuthenticateV0_Wif/Tests/Unit/Unit.npmrcCredential.ts | New unit tests for endpoint credential resolution logic. |
| _generated/NpmAuthenticateV0_Wif/Tests/Unit/Unit.npmrcBackupManager.ts | New unit tests for backup/restore behavior. |
| _generated/NpmAuthenticateV0_Wif/Tests/Unit/Unit.npmauthutils.ts | New unit tests for normalization/nerf-dart and npmrc mutations. |
| _generated/NpmAuthenticateV0_Wif/Tests/Unit.ts | Unit test entrypoint for new pure-logic tests. |
| _generated/NpmAuthenticateV0_Wif/Tests/TestSetupEndpointCredential.ts | New integration-style setup to exercise real endpoint resolution via env vars. |
| _generated/NpmAuthenticateV0_Wif/Tests/TestSetupCleanup.ts | Updates cleanup test setup to remove packaging-common util mock. |
| _generated/NpmAuthenticateV0_Wif/Tests/TestSetup.ts | Updates main test setup to align with new utilities and internal registry discovery. |
| _generated/NpmAuthenticateV0_Wif/Tests/TestHelpers.ts | Updates helpers to assert from disk .npmrc content rather than mocked append logs. |
| _generated/NpmAuthenticateV0_Wif/Tests/TestConstants.ts | Adds env vars for endpoint probe/error-path test scenarios. |
| _generated/NpmAuthenticateV0_Wif/Tests/L0.ts | Adds new L0 suites for endpoint credential resolution and error paths (WIF build). |
| _generated/NpmAuthenticateV0_Wif/Tests/L0.WIF.ts | Updates WIF tests to assert file content and adds feedUrl normalization tests. |
| _generated/NpmAuthenticateV0_Wif/Tests/L0.Telemetry.ts | Updates telemetry tests to rely on internal feed host matching rather than mocks. |
| _generated/NpmAuthenticateV0_Wif/Tests/L0.ErrorPaths.ts | Adds L0 coverage for packaging location failures and HTTP probe fallback behavior. |
| _generated/NpmAuthenticateV0_Wif/Tests/L0.EndpointCredential.ts | Adds L0 coverage for “real” endpoint credential resolution paths. |
| _generated/NpmAuthenticateV0_Wif/Tests/L0.Cleanup.ts | Updates cleanup tests for the new backup file format and restore semantics. |
| _generated/NpmAuthenticateV0_Wif/Strings/resources.resjson/en-US/resources.resjson | Adds localized string for InvalidRegistryUrl. |
| _generated/NpmAuthenticateV0/task.loc.json | Version bump + adds InvalidRegistryUrl message mapping. |
| _generated/NpmAuthenticateV0/task.json | Version bump + adds InvalidRegistryUrl message. |
| _generated/NpmAuthenticateV0/package.json | Adds ini dependency for local npmrc parsing. |
| _generated/NpmAuthenticateV0/package-lock.json | Locks ini dependency. |
| _generated/NpmAuthenticateV0/npmrcCredential.ts | Adds local endpoint credential resolution + internal/external probing. |
| _generated/NpmAuthenticateV0/npmrcBackupManager.ts | Adds local backup/restore manager for .npmrc files. |
| _generated/NpmAuthenticateV0/npmauthutils.ts | Adds local helpers for registry normalization, parsing, auth appends, and cleanup. |
| _generated/NpmAuthenticateV0/npmauthcleanup.ts | Switches cleanup from packaging-common util to NpmrcBackupManager. |
| _generated/NpmAuthenticateV0/npmauth.ts | Switches main task logic to use npmauthutils instead of packaging-common npm modules. |
| _generated/NpmAuthenticateV0/Tests/Unit/Unit.npmrcCredential.ts | New unit tests for endpoint credential resolution logic. |
| _generated/NpmAuthenticateV0/Tests/Unit/Unit.npmrcBackupManager.ts | New unit tests for backup/restore behavior. |
| _generated/NpmAuthenticateV0/Tests/Unit/Unit.npmauthutils.ts | New unit tests for normalization/nerf-dart and npmrc mutations. |
| _generated/NpmAuthenticateV0/Tests/Unit.ts | Unit test entrypoint for new pure-logic tests. |
| _generated/NpmAuthenticateV0/Tests/TestSetupEndpointCredential.ts | New integration-style setup to exercise real endpoint resolution via env vars. |
| _generated/NpmAuthenticateV0/Tests/TestSetupCleanup.ts | Updates cleanup test setup to remove packaging-common util mock. |
| _generated/NpmAuthenticateV0/Tests/TestSetup.ts | Updates main test setup to align with new utilities and internal registry discovery. |
| _generated/NpmAuthenticateV0/Tests/TestHelpers.ts | Updates helpers to assert from disk .npmrc content rather than mocked append logs. |
| _generated/NpmAuthenticateV0/Tests/TestConstants.ts | Adds env vars for endpoint probe/error-path test scenarios. |
| _generated/NpmAuthenticateV0/Tests/L0.ts | Adds new L0 suites for endpoint credential resolution and error paths. |
| _generated/NpmAuthenticateV0/Tests/L0.WIF.ts | Updates WIF tests to assert file content and adds feedUrl normalization tests. |
| _generated/NpmAuthenticateV0/Tests/L0.Telemetry.ts | Updates telemetry tests to rely on internal feed host matching rather than mocks. |
| _generated/NpmAuthenticateV0/Tests/L0.ErrorPaths.ts | Adds L0 coverage for packaging location failures and HTTP probe fallback behavior. |
| _generated/NpmAuthenticateV0/Tests/L0.EndpointCredential.ts | Adds L0 coverage for “real” endpoint credential resolution paths. |
| _generated/NpmAuthenticateV0/Tests/L0.Cleanup.ts | Updates cleanup tests for the new backup file format and restore semantics. |
| _generated/NpmAuthenticateV0/Strings/resources.resjson/en-US/resources.resjson | Adds localized string for InvalidRegistryUrl. |
| _generated/NpmAuthenticateV0.versionmap.txt | Updates generated version map to new Default/WIF versions. |
| Tasks/NpmAuthenticateV0/task.loc.json | Bumps task version and adds InvalidRegistryUrl message mapping (source). |
| Tasks/NpmAuthenticateV0/task.json | Bumps task version and adds InvalidRegistryUrl message (source). |
| Tasks/NpmAuthenticateV0/package.json | Adds ini dependency for local parsing (source). |
| Tasks/NpmAuthenticateV0/package-lock.json | Locks dependency graph for the task (source). |
| Tasks/NpmAuthenticateV0/npmrcCredential.ts | Adds local endpoint credential resolution + probe logic (source). |
| Tasks/NpmAuthenticateV0/npmrcBackupManager.ts | Adds local backup/restore manager (source). |
| Tasks/NpmAuthenticateV0/npmauthutils.ts | Adds local registry parsing/mutation helpers and packaging location helper (source). |
| Tasks/NpmAuthenticateV0/npmauthcleanup.ts | Updates cleanup to use NpmrcBackupManager (source). |
| Tasks/NpmAuthenticateV0/_buildConfigs/Wif/package.json | Adds ini dependency for WIF build config. |
| Tasks/NpmAuthenticateV0/_buildConfigs/Wif/package-lock.json | Locks dependency graph for WIF build config. |
| Tasks/NpmAuthenticateV0/Tests/Unit/Unit.npmrcCredential.ts | Adds unit tests for endpoint credential behavior (source tests). |
| Tasks/NpmAuthenticateV0/Tests/Unit/Unit.npmrcBackupManager.ts | Adds unit tests for backup/restore behavior (source tests). |
| Tasks/NpmAuthenticateV0/Tests/Unit/Unit.npmauthutils.ts | Adds unit tests for parsing/normalization and file mutations (source tests). |
| Tasks/NpmAuthenticateV0/Tests/Unit.ts | Unit test suite entrypoint (source tests). |
| Tasks/NpmAuthenticateV0/Tests/TestSetupEndpointCredential.ts | Test setup for exercising real endpoint credential logic (source tests). |
| Tasks/NpmAuthenticateV0/Tests/TestSetupCleanup.ts | Updates cleanup test setup for new backup mechanism (source tests). |
| Tasks/NpmAuthenticateV0/Tests/TestSetup.ts | Updates main test setup to reflect new utilities (source tests). |
| Tasks/NpmAuthenticateV0/Tests/TestHelpers.ts | Updates helper assertions to validate actual .npmrc content on disk (source tests). |
| Tasks/NpmAuthenticateV0/Tests/TestConstants.ts | Adds new env vars used by endpoint/error-path tests (source tests). |
| Tasks/NpmAuthenticateV0/Tests/L0.ts | Adds L0 suites for endpoint credentials and error paths (source tests). |
| Tasks/NpmAuthenticateV0/Tests/L0.WIF.ts | Updates WIF L0 suite and adds feedUrl normalization cases (source tests). |
| Tasks/NpmAuthenticateV0/Tests/L0.Telemetry.ts | Updates telemetry L0 suite to align with new internal registry resolution (source tests). |
| Tasks/NpmAuthenticateV0/Tests/L0.ErrorPaths.ts | Adds L0 error-path coverage for packaging location and probe fallback (source tests). |
| Tasks/NpmAuthenticateV0/Tests/L0.EndpointCredential.ts | Adds L0 coverage for endpoint credential resolution variations (source tests). |
| Tasks/NpmAuthenticateV0/Tests/L0.Cleanup.ts | Updates L0 cleanup tests for new backup index/backup file layout (source tests). |
| Tasks/NpmAuthenticateV0/Tests/L0.Authentication.ts | Updates integration auth tests to reflect new internal/external credential flows (source tests). |
Files not reviewed (4)
- Tasks/NpmAuthenticateV0/_buildConfigs/Wif/package-lock.json: Language not supported
- Tasks/NpmAuthenticateV0/package-lock.json: Language not supported
- _generated/NpmAuthenticateV0/package-lock.json: Language not supported
- _generated/NpmAuthenticateV0_Wif/package-lock.json: Language not supported
Comments suppressed due to low confidence (2)
Tasks/NpmAuthenticateV0/package.json:29
isEndpointInternalusesrequire('https-proxy-agent'), buthttps-proxy-agentis not declared as a direct dependency in this task’spackage.json(it currently arrives transitively). This makes the runtime brittle if upstream dependencies change; please add it explicitly (and to the WIF build config) or remove the direct require in favor of a dependency you already declare.
"dependencies": {
"azure-pipelines-task-lib": "^5.2.4",
"azure-pipelines-tasks-packaging-common": "^3.270.0",
"azure-pipelines-tasks-artifacts-common": "^2.270.0",
"ini": "^1.3.8",
"@types/mocha": "^5.2.7",
"@types/node": "^24.10.0",
"@types/uuid": "^8.3.0"
},
Tasks/NpmAuthenticateV0/_buildConfigs/Wif/package.json:27
- Same issue as the root task package: code requires
https-proxy-agentbut this WIF build config doesn’t list it as a direct dependency. Please addhttps-proxy-agenthere as well (or remove the direct require).
"dependencies": {
"azure-pipelines-task-lib": "^5.2.4",
"azure-pipelines-tasks-packaging-common": "^3.270.0",
"azure-pipelines-tasks-artifacts-common": "^2.270.0",
"ini": "^1.3.8",
"@types/mocha": "^5.2.7",
"@types/node": "^24.10.0",
"@types/uuid": "^8.3.0"
},
You can also share your feedback on Copilot code review. Take the survey.
MicroTuld
left a comment
There was a problem hiding this comment.
Also, in constants.ts, CustomRegistry is unused (can't comment in the file because the file was untouched?)
MicroTuld
left a comment
There was a problem hiding this comment.
Left a bunch of comments
fix false warnings add unit tests Fix double error throwing Remove dead code Move WIF token check to after vars are validated
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
…github.com/microsoft/azure-pipelines-tasks into users/alextorres/NpmAuthenticateV0Refactor
|
/azp run |
1 similar comment
|
/azp run |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
MicroTuld
left a comment
There was a problem hiding this comment.
More comments, most innocuous
| try { | ||
| const proxy = tl.getHttpProxyConfiguration(); | ||
| if (proxy) { | ||
| options.agent = new (require('https-proxy-agent'))(proxy.proxyUrl); |
There was a problem hiding this comment.
You could add https-proxy-agent to package.json as a direct dependency, import the module at the top of the file, then dispense with the try/catch block, rather than use require().
| let addedRegistry = []; | ||
| let npmrcRegistries = npmrcparser.GetRegistries(npmrc, /* saveNormalizedRegistries */ true); | ||
| let addedRegistries: URL[] = []; | ||
| let npmrcRegistries = npmauthutils.getRegistriesFromNpmrc(npmrc); |
There was a problem hiding this comment.
This looks like it reads from the user-specified .npmrc file, rather than the project .npmrc file like in npmauth.ts resolveInternalFeedCredentials()
| } | ||
|
|
||
| // Write normalized URLs back so downstream npm/yarn sees trailing slashes. | ||
| tl.writeFile(npmrcPath, ini.stringify(config)); |
There was a problem hiding this comment.
The file is rewritten before a backup has been created, allowing for the possibility of permanently altering the file.
| } | ||
|
|
||
| const registries: NpmrcCredential[] = []; | ||
| await Promise.all(endpointIds.map(async (endpointId) => { |
There was a problem hiding this comment.
NIT: Might wish to avoid the any type here.
| "azure-pipelines-tasks-packaging-common": "^3.270.0", | ||
| "azure-pipelines-tasks-artifacts-common": "^2.270.0", | ||
| "ini": "^1.3.8", | ||
| "@types/mocha": "^5.2.7", |
There was a problem hiding this comment.
@types dependencies should be in devDependencies, since they are not needed for runtime.
| isInternalEndpoint: '__npmauth_isInternalEndpoint__', | ||
| packagingLocationShouldFail: '__npmauth_packagingLocationShouldFail__', | ||
| httpProbeShouldFail: '__npmauth_httpProbeShouldFail__' | ||
| }; |
There was a problem hiding this comment.
GitHub won't let me comment a file where it did not change ---
appendPrefix and localRegistries are no longer in use.
| for (let RegistryURLString of npmrcRegistries) { | ||
| let registryURL = URL.parse(RegistryURLString); | ||
| let registry: npmregistry.NpmRegistry; | ||
| for (const RegistryURLString of npmrcRegistries) { |
| options.agent = new (require('https-proxy-agent'))(proxy.proxyUrl); | ||
| } | ||
| } catch { | ||
| tl.debug('Unable to determine proxy configuration'); |
There was a problem hiding this comment.
Should be consistent on whether debug output is localized.
| }); | ||
|
|
||
| req.on('timeout', () => { | ||
| tl.debug(`isEndpointInternal timed out after ${TIMEOUT_MS}ms`); |
There was a problem hiding this comment.
Should be consistent on whether debug output is localized.
| }); | ||
|
|
||
| req.on('error', (error) => { | ||
| tl.debug(`isEndpointInternal check failed: ${error}`); |
There was a problem hiding this comment.
Should be consistent on whether debug output is localized.
Context
packaging-common/npm/ is a shared package consumed by three tasks: NpmAuthenticateV0, NpmV1, and DownloadGitHubNpmPackageV1. Despite being "shared," each task uses a different subset of the package. The shared abstraction adds dependency coupling and test complexity without meaningful code reuse. This PR will decouple the dependency on a package from the separate repo, while providing the same utility in an improved manner.
AB#{2364282}
Task Name
NpmAuthenticateV0
Description
What changed:
Inlined registry parsing, URL normalization, credential formatting, and file I/O (~120 lines replacing 3 external module imports)
Replaced the NpmRegistry class with a local NpmrcCredential interface
Extracted credential resolution into npmrcCredential.ts and backup management into npmrcBackupManager.ts
Consolidated the duplicated WIF auth branches into a single check
Improved variable naming throughout (endpointsArray → previouslyAuthenticatedUrls, registry → npmrcEntry, etc.)
Rewrote tests to exercise real code paths instead of mocking our own logic
The only remaining packaging-common dependency is locationUtilities (for Azure DevOps service location), which is a general-purpose module used across many tasks.
Risk Assessment (Low / Medium / High)
Medium
Change Behind Feature Flag (Yes / No)
Can this change be behine feature flag, if not why?
N - these are primarily utility method changes that cannot be hidden behind a FF
Tech Design / Approach
Documentation Changes Required (Yes/No)
Indicate whether related documentation needs to be updated.
N/A
Unit Tests Added or Updated (Yes / No)
Indicate whether unit tests were added or modified to reflect these changes.
Y
Additional Testing Performed
List all other tests performed (manual or automated, including integration, regression, scenario tests, etc.).
Local, E2E, Testing Pipelines
Unit Tests, L0s, Testing Pipelines, Devfabric
Logging Added/Updated (Yes/No)
Y
Telemetry Added/Updated (Yes/No)
N
Rollback Scenario and Process (Yes/No)
Rollback via task override can be applied to revert to a previous version
Rollback can be initiated via Task Overriding to a previous version: https://dev.azure.com/mseng/AzureDevOps/_wiki/wikis/AzureDevOps.wiki/51347/Task-Deployment-Guide-Hotfix
Dependency Impact Assessed and Regression Tested (Yes/No)
Y
Checklist