Set TYPESPEC_NPM_REGISTRY to devOps feed - #11674
Set TYPESPEC_NPM_REGISTRY to devOps feed#11674Chidozie Ononiwu (chidozieononiwu) wants to merge 6 commits into
Conversation
|
❌ There is undocummented changes. Run The following packages have changes but are not documented.
Show changes |
|
You can try these changes here
|
commit: |
a73a488 to
63a57ac
Compare
2165c3b to
15ec79d
Compare
15ec79d to
28ad7f4
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the TypeSpec compiler’s npm-registry interactions to work with Azure DevOps npm feeds (including auth handling and packument-based version resolution), and wires the CI E2E job to use a DevOps feed via TYPESPEC_NPM_REGISTRY.
Changes:
- Add
.npmrcparsing to load registry + auth (Bearer / Basic) and apply scopedAuthorizationheaders to registry/tarball requests. - Add Azure DevOps feed support by fetching the packument (
/<pkg>) and resolving dist-tags/versions from it. - Configure the E2E pipeline job to use the Azure DevOps npm registry feed (and adjust init template snapshots accordingly).
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/compiler/test/package-manager/npm-registry.test.ts | Expands coverage for Azure DevOps packuments, error handling, and auth header scoping. |
| packages/compiler/templates/snapshots/rest/package.json | Updates expected scaffolded dependency versions under the new registry behavior. |
| packages/compiler/templates/snapshots/empty/package.json | Updates expected scaffolded dependency versions under the new registry behavior. |
| packages/compiler/src/package-manger/npm-registry.ts | Adds registry config plumbing, Azure DevOps packument resolution, and auth header selection. |
| packages/compiler/src/package-manger/npm-registry-config.ts | New helper to load registry/auth settings from .npmrc. |
| packages/compiler/src/package-manger/npm-package-download.ts | Loads npm registry config and applies auth headers to tarball downloads. |
| packages/compiler/src/install/install.ts | Plumbs loaded npm registry config into manifest fetch + tarball extraction for package manager install. |
| packages/compiler/src/init/scaffold.ts | Allows init scaffolding to resolve versions using the configured registry/auth. |
| packages/compiler/src/init/init.ts | Loads npm registry config during init and passes it through scaffolding. |
| eng/tsp-core/pipelines/jobs/e2e.yml | Sets TYPESPEC_NPM_REGISTRY to the Azure DevOps feed for E2E runs. |
| .npmrc | Removes repo-level npm configuration. |
Suppressed comments (2)
packages/compiler/test/package-manager/npm-registry.test.ts:63
- afterEach deletes TYPESPEC_NPM_REGISTRY and NPM_CONFIG_USERCONFIG unconditionally. This can break test isolation when those env vars were already set before this file ran (e.g., by the CI job). Prefer restoring the original values captured at module load (similar to HOME/USERPROFILE).
afterEach(async () => {
delete process.env["TYPESPEC_NPM_REGISTRY"];
delete process.env["NPM_CONFIG_USERCONFIG"];
restoreEnvironmentVariable("HOME", originalHome);
restoreEnvironmentVariable("USERPROFILE", originalUserProfile);
.npmrc:1
- This PR description says it’s a small CI pipeline change, but it also removes the repo root .npmrc entirely. That drops settings like engine-strict and manage-package-manager-versions for all contributors and CI runs that rely on repo-level npm config. If this removal is not intentional, restore the file; otherwise, update the PR description to reflect the broader behavioral change.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| let tempDirectory: string | undefined; | ||
| const originalHome = process.env["HOME"]; | ||
| const originalUserProfile = process.env["USERPROFILE"]; | ||
|
|
| const res = await fetch(url, { headers: getNpmRequestHeaders(url, config) }); | ||
| const tarballStream = Readable.fromWeb(res.body as any); | ||
| const hash = tarballStream.pipe(createHash(hashAlgorithm)); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (2)
packages/compiler/src/package-manger/npm-package-download.ts:51
downloadAndExtractTarballdoesn’t checkres.ok(orres.body) before streaming/extracting. With the new registry/auth support, 401/403/404 responses are more likely, and this will currently surface as a cryptic stream/tar error rather than a clear HTTP failure.
const res = await fetch(url, { headers: getNpmRequestHeaders(url, config) });
const tarballStream = Readable.fromWeb(res.body as any);
const hash = tarballStream.pipe(createHash(hashAlgorithm));
eng/tsp-core/pipelines/publish.yml:36
- The PR title/description says this is a small CI change adding
TYPESPEC_NPM_REGISTRY, but this change also enables the E2E job in the publish pipeline (and the PR includes substantial compiler/npm-registry behavior changes elsewhere). This mismatch makes review/release risk harder to assess—please update the PR metadata to reflect the full scope, or split CI-only changes from compiler changes.
- template: /eng/tsp-core/pipelines/jobs/build-packages.yml@self
- template: /eng/tsp-core/pipelines/jobs/build-vs.yml@self
- template: /eng/tsp-core/pipelines/jobs/cli/build-tsp-cli-all.yml@self
- template: /eng/tsp-core/pipelines/jobs/e2e.yml@self
parameters:
azLogin: true
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (5)
packages/compiler/src/package-manger/npm-package-download.ts:50
downloadAndExtractTarballstreamsres.bodywithout verifying the response succeeded. With registry auth now supported, a 401/403/404 would currently surface as a cryptic stream/untar failure (orres.bodycould be null). Add an explicit status check (and body null check) before creating the stream so failures are reported clearly.
const res = await fetch(url, { headers: getNpmRequestHeaders(url, config) });
const tarballStream = Readable.fromWeb(res.body as any);
packages/compiler/src/package-manger/npm-registry.ts:120
fetchPackageManifesttreats a missing/undefinedres.okas a failed request because it usesif (!res.ok). This breaks commonfetchmocks that only implementjson()(e.g., template snapshot tests), and it also differs from the intent of checking for an explicit non-OK response. Consider checkingres.ok === falseinstead so only an explicitfalsetriggers the error path.
const res = await fetch(url, { headers: getNpmRequestHeaders(url, config) });
if (!res.ok) {
throw new Error(`Request to ${url} failed with status ${res.status}.`);
}
packages/compiler/templates/snapshots/empty/package.json:7
- These init-template snapshots now pin
@typespec/compilerto"latest", which makes the snapshot less deterministic and inconsistent with the snapshot-generation test’s mocked registry response (it returns version1.0.0, so scaffolding should resolve to^1.0.0). This change is likely a side-effect of the new registry fetch error path rather than an intentional template behavior change.
"@typespec/compiler": "latest"
packages/compiler/templates/snapshots/rest/package.json:11
- These init-template snapshots now pin all
@typespecdependencies to"latest", which makes the snapshot less deterministic and inconsistent with the snapshot-generation test’s mocked registry response (it returns version1.0.0, so scaffolding should resolve to^1.0.0). This looks like an unintended side effect rather than an intentional template update.
"@typespec/compiler": "latest",
"@typespec/http": "latest",
"@typespec/rest": "latest",
"@typespec/openapi": "latest",
"@typespec/openapi3": "latest"
eng/tsp-core/pipelines/publish.yml:36
- The PR description says this is a small CI change to add
TYPESPEC_NPM_REGISTRY, but this file also re-enables the E2E job in the publish pipeline, which materially changes CI coverage and runtime/cost. Either update the PR description to include this behavior change (and its motivation), or split it into a separate PR so the registry change can be reviewed independently.
- template: /eng/tsp-core/pipelines/jobs/build-packages.yml@self
- template: /eng/tsp-core/pipelines/jobs/build-vs.yml@self
- template: /eng/tsp-core/pipelines/jobs/cli/build-tsp-cli-all.yml@self
- template: /eng/tsp-core/pipelines/jobs/e2e.yml@self
parameters:
azLogin: true
07f82df to
17ba9cd
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
packages/compiler/src/package-manger/npm-package-download.ts:50
downloadAndExtractTarballdoesn't checkres.ok(or thatres.bodyis present) before streaming/extracting. If auth is missing/invalid, this will try to untar an error response (or crash on a null body) and produce a confusing failure. Fail fast with a clear error when the fetch fails.
const res = await fetch(url, { headers: getNpmRequestHeaders(url, config) });
const tarballStream = Readable.fromWeb(res.body as any);
eng/tsp-core/pipelines/publish.yml:36
- This PR is described as only adding
TYPESPEC_NPM_REGISTRYto the CI configuration, but this change also enables the E2E job in the publish pipeline. That alters pipeline behavior/cost and should either be called out explicitly in the PR description or split into a separate PR to keep the change scoped.
- template: /eng/tsp-core/pipelines/jobs/build-packages.yml@self
- template: /eng/tsp-core/pipelines/jobs/build-vs.yml@self
- template: /eng/tsp-core/pipelines/jobs/cli/build-tsp-cli-all.yml@self
- template: /eng/tsp-core/pipelines/jobs/e2e.yml@self
parameters:
azLogin: true
| _shrinkwrap: null, | ||
| }), | ||
| JSON.stringify( | ||
| lastRequestUrl.includes("/_packaging/") |
| directory, | ||
| parameters, | ||
| emitters, | ||
| npmRegistryConfig: await loadNpmRegistryConfig(), |
There was a problem hiding this comment.
I think the compiler changes need to wait for some input from Timothee Guerin (@timotheeguerin)
This pull request makes a small change to the CI pipeline configuration. It adds the
TYPESPEC_NPM_REGISTRYvariable to specify the npm registry used for builds.