-
Notifications
You must be signed in to change notification settings - Fork 562
Use internal repository to manage secondary pipeline dependencies #25535
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
Conversation
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.
Pull Request Overview
This PR modernizes dependency management for secondary pipelines by introducing an internal repository approach. The change moves from using npx for direct package installations to utilizing the ff_pipeline_host repository with pnpm and lockfile-based dependency management.
Key changes include:
- Migrating from
npxcommands topnpm execfor better dependency control - Switching from custom workspace directories to
$(Build.SourcesDirectory) - Adding proper dependency installation steps using pnpm with lockfiles
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tools/pipelines/templates/include-vars-telemetry-generator.yml | Adds new variable for telemetry generator handlers path in the repository-based approach |
| tools/pipelines/templates/include-upload-stage-telemetry.yml | Replaces telemetry setup template with repository checkout and pnpm-based dependency installation |
| tools/pipelines/templates/include-test-real-service.yml | Major refactor from workspace-based npm to repository-based pnpm approach with streamlined dependency management |
| scripts/get-test-pass-rate.mjs | Removes script file (likely moved to the internal repository) |
| // This script is used in the "runAfterAll" stage in our E2E test pipeline. It's used | ||
| // to get timeline and metrics data so that the test pass rate can be calculated in a later step. | ||
|
|
||
| // The Build ID needed to fetch the desired data. |
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.
I just moved this whole script to the internal repository. If we leave it here, we need another checkout step in the telemetry upload job to get at it, which seems kinda wasteful.
Moving it comes with some more compatibility concerns, but I think that's probably ok.
| workingDirectory: ${{ parameters.testWorkspace }} | ||
| targetType: 'inline' | ||
| script: 'npm install $(Initialize.testPackageTgz) ${{ parameters.loggerPackage }}' | ||
| script: 'pnpm install $(Initialize.testPackageTgz)' |
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.
Is the logger installed by default when the pipeline host repo is checked out?
jzaffiro
left a comment
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.
I think the performance benchmarks pipeline will need to be updated separately since it doesn't use the include-test-real-service template, but I'm fine to have this go in first.
Yup, it definitely needs updates. That's the one I think might be broken as well by this refactor, but we'll see. It's broken now anyway, so... :P |
## Description Follow-up to #25535: this extra syntax is no longer required after moving off `npx` and though they are benign for me locally, they cause parameters to not be plumbed through in pipelines (likely due to shell differences) Test run: [here](https://dev.azure.com/fluidframework/internal/_build/results?buildId=357111&view=results) Co-authored-by: Abram Sanderson <[email protected]>
## Description Follow-up to #25535 that allows specifying the primary registry in `include-install-pnpm.yaml` so that the telemetry stage upload template can reference the feed we use to install internal packages. Test run [here](https://dev.azure.com/fluidframework/internal/_build/results?buildId=357059&view=logs&j=7ef2f12f-27d7-5730-657b-d3ad5f630362&t=9da23dd1-2e0f-578e-ea27-ee9162e0f22c). Co-authored-by: Abram Sanderson <[email protected]>
…25543) ## Description Follow-up to #25535: in line with using a "host repository" that has package-lock'd internal dependencies, tighten the "copy devDependency" script that we use to get devDependencies necessary at workload runtime to exclude some common dependencies that are only used at build time. This is motivated by still seeing some DDS fuzz harness pipeline failures due to recently published transitive dependencies of build-tools. Test run: [here](https://dev.azure.com/fluidframework/internal/_build/results?buildId=357194&view=logs&j=bbaafa3d-c204-5f81-4508-786dc29a3642&t=f302f968-fb15-5cb8-d0fb-0d951f384806) --------- Co-authored-by: Abram Sanderson <[email protected]>
…25550) ## Description Follow-up to #25535 which applies the same kind of change to the performance benchmarks pipeline template. Test run: [here](https://dev.azure.com/fluidframework/internal/_build/results?buildId=357522&view=results) --------- Co-authored-by: Abram Sanderson <[email protected]>
) ## Description Follow-up to #25535 which restores the caching of compat dependencies we previously had. Couldn't help myself from making a tweak so the cache key is invalidated on minor version change, which should give us better long-term behavior: the set of versions to install is dynamically computed based on roughly the current package version, so using major+minor here should be a net win as only roughly one build per release should need to install compat dependencies, after that they will be cached. --------- Co-authored-by: Abram Sanderson <[email protected]>
…crosoft#25535) ## Description Our secondary pipelines (real service e2e, stress tests, performance benchmarks, etc.) rely on a small amount of infrastructure published internally as npm packages. Before this change, we have been consuming those packages using `npx` or equivalent bare installations from within the secondary pipeline. This is not great practice as it does not allow repeatability of the generated install tree, as npm and other package managers prefer to use newer versions of packages within semver allowance as they are released. This PR updates the `include-real-service-test` template (which is used by several of these pipelines) to run its workload from the context of a very simple internal repository which has pnpm configurations and the packages we need installed. This gives us the benefits of a lock file within source control. Test run: [here](https://dev.azure.com/fluidframework/internal/_build/results?buildId=356944&view=results) --------- Co-authored-by: Abram Sanderson <[email protected]>
## Description Follow-up to microsoft#25535: this extra syntax is no longer required after moving off `npx` and though they are benign for me locally, they cause parameters to not be plumbed through in pipelines (likely due to shell differences) Test run: [here](https://dev.azure.com/fluidframework/internal/_build/results?buildId=357111&view=results) Co-authored-by: Abram Sanderson <[email protected]>
…soft#25540) ## Description Follow-up to microsoft#25535 that allows specifying the primary registry in `include-install-pnpm.yaml` so that the telemetry stage upload template can reference the feed we use to install internal packages. Test run [here](https://dev.azure.com/fluidframework/internal/_build/results?buildId=357059&view=logs&j=7ef2f12f-27d7-5730-657b-d3ad5f630362&t=9da23dd1-2e0f-578e-ea27-ee9162e0f22c). Co-authored-by: Abram Sanderson <[email protected]>
…icrosoft#25543) ## Description Follow-up to microsoft#25535: in line with using a "host repository" that has package-lock'd internal dependencies, tighten the "copy devDependency" script that we use to get devDependencies necessary at workload runtime to exclude some common dependencies that are only used at build time. This is motivated by still seeing some DDS fuzz harness pipeline failures due to recently published transitive dependencies of build-tools. Test run: [here](https://dev.azure.com/fluidframework/internal/_build/results?buildId=357194&view=logs&j=bbaafa3d-c204-5f81-4508-786dc29a3642&t=f302f968-fb15-5cb8-d0fb-0d951f384806) --------- Co-authored-by: Abram Sanderson <[email protected]>
…icrosoft#25550) ## Description Follow-up to microsoft#25535 which applies the same kind of change to the performance benchmarks pipeline template. Test run: [here](https://dev.azure.com/fluidframework/internal/_build/results?buildId=357522&view=results) --------- Co-authored-by: Abram Sanderson <[email protected]>
…rosoft#25563) ## Description Follow-up to microsoft#25535 which restores the caching of compat dependencies we previously had. Couldn't help myself from making a tweak so the cache key is invalidated on minor version change, which should give us better long-term behavior: the set of versions to install is dynamically computed based on roughly the current package version, so using major+minor here should be a net win as only roughly one build per release should need to install compat dependencies, after that they will be cached. --------- Co-authored-by: Abram Sanderson <[email protected]>
Description
Our secondary pipelines (real service e2e, stress tests, performance benchmarks, etc.) rely on a small amount of infrastructure published internally as npm packages. Before this change, we have been consuming those packages using
npxor equivalent bare installations from within the secondary pipeline. This is not great practice as it does not allow repeatability of the generated install tree, as npm and other package managers prefer to use newer versions of packages within semver allowance as they are released.This PR updates the
include-real-service-testtemplate (which is used by several of these pipelines) to run its workload from the context of a very simple internal repository which has pnpm configurations and the packages we need installed. This gives us the benefits of a lock file within source control.Test run: here