Skip to content
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

feat: stop ADO extension reliance on pipeline-installed Node and Yarn versions (by migrating to yarn v3) #1559

Merged
merged 14 commits into from Mar 1, 2023

Conversation

dbjorge
Copy link
Contributor

@dbjorge dbjorge commented Feb 24, 2023

Details

Leaving this as a draft pending feedback on design + testing as-deployed in a test extension

This PR migrates the repo to yarn v3, both for development of the extension and also for our customers. Notable impacts of this are:

  • A copy of yarn 3 is now checked into the repo
  • The same copy of yarn 3 is now bundled with the ADO extension and invoked directly. This means that users will no longer need to include any explicit npm install -g yarn step, and it insulates us from issues due to from differences in yarn versions based on customer environment.
  • Because of this above, it is easier to invoke our bundled yarn using the same node binary that our script runs within (the node 16 version guaranteed to exist on the agent regardless of user pipeline tasks). This means that users will no longer need to include any explicit NodeTools@0 step, and it insulates us from issues due to differences in Node versions based on customer environment (I suspect this should fix Accessibility-insights for azure devops not working and throw exception #1551).
    • This has the very significant downside of relying on the agent node version to be recent enough to support our task. That's true today, but we've had issues with that in the past - agents didn't add node16 support for tasks until well after we were having trouble with the previously-supported version being EOL and unsupported by many of our dependencies
  • We get to drop our lerna dependency in favor of the yarn workspaces plugin, which substantially reduces our overall dependency tree size
  • Pre-empts the possibility of package-squatting attacks against workspace-internal packages
Motivation
  • Removes a few steps from onboarding
  • Prevents several classes of errors we've seen customers hit recently
  • Enables more precise yarn resolutions
  • Pre-empts package-squatting attacks
Context

Reviewer note: Most of the individual changes are more or less mechanical and not super interesting. However, these three files are substantive changes to the way the extension functions and need deeper review:

  • packages/ado-extension/prepare-package-dir.js
  • packages/ado-extension/src/install-runtime-dependencies.ts
  • packages/shared/build-runtime-package-metadata.js

Some reference material I used while putting together this PR:

Pull request checklist

@dbjorge dbjorge changed the title feat: migrate from global yarn v1 to bundled yarn v3 feat: stop ADO extension reliance on globally installed Node and Yarn versions (by migrating to yarn v3) Feb 24, 2023
@dbjorge dbjorge changed the title feat: stop ADO extension reliance on globally installed Node and Yarn versions (by migrating to yarn v3) feat: stop ADO extension reliance on pipeline-installed Node and Yarn versions (by migrating to yarn v3) Feb 24, 2023
@@ -55,7 +57,7 @@
"jest-extended": "^3.2.4",
"jest-file-snapshot": "^0.5.0",
"jest-junit": "^15.0.0",
"lerna": "^6.5.1",
"js-yaml": "^4.1.0",
Copy link
Contributor Author

@dbjorge dbjorge Feb 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

js-yaml is here because bundling a working .yarnrc.yml required manipulating our base one a bit at build time. I picked this over other yaml libraries (particularly, yaml) by asking "among widely-used and recently-maintained options, which one do we already have the latest version of in our transitive dependency tree anyway?"

@katydecorah
Copy link
Contributor

@dbjorge Overall, I think this would be a big improvement to the onboarding flow!

A few small questions:

  • What does the maintenance of having yarn checked into the repo look like? Should we expect to update this periodically?
  • Curiosity question: I noticed that the package.json files now have added devDependencies, was that a result of switching from lerna to yarn 3?
  • I think this pull request could remove lerna.json at the root, correct?

@dbjorge
Copy link
Contributor Author

dbjorge commented Feb 28, 2023

  • What does the maintenance of having yarn checked into the repo look like? Should we expect to update this periodically?

Yes, we should expect to update this periodically, probably at a pace comparable to updating node versions we use.

  • Curiosity question: I noticed that the package.json files now have added devDependencies, was that a result of switching from lerna to yarn 3?

This is a result of yarn v2+ being more strict about importing/using dependencies that aren't explicitly listed. Particularly, yarn v2+ doesn't place entries in packages/subpackage/node_modules/.bin for devDependencies that aren't explicitly listed by the subpackage, which breaks most dev tools we use in practice.

  • I think this pull request could remove lerna.json at the root, correct?

Yep, good catch, will push an update.

@dbjorge
Copy link
Contributor Author

dbjorge commented Feb 28, 2023

Test run against 8e89098. Ran a matrix of test cases (all combinations of hosted agent images ubuntu-18.04, ubuntu-20.04, ubuntu-22.04, windows-2019, windows-2022 each with all combinations of "no explicit node/yarn install steps", "install node 16", "install node 16 + yarn", "install node 18"), all worked as expected.

@dbjorge dbjorge marked this pull request as ready for review February 28, 2023 23:14
@dbjorge dbjorge requested a review from a team as a code owner February 28, 2023 23:14
Copy link
Contributor

@katydecorah katydecorah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be a big onboarding improvement! Nice work, @dbjorge!

@microsoft-github-policy-service microsoft-github-policy-service bot merged commit 52050ad into microsoft:main Mar 1, 2023
@dbjorge dbjorge deleted the bundle-yarn branch March 1, 2023 16:59
dbjorge added a commit to microsoft/accessibility-insights-web that referenced this pull request Mar 22, 2023
#### Details

Migrates the repo's package manager from yarn v1 to yarn v3 (in
non-zero-installs configuration)

Some of this was mechanical based on [Yarn's migration
guide](https://yarnpkg.com/getting-started/migration). Beyond the steps
there, this PR:

* Updates assorted CI pipelines/workflows and dev docs to adapt to minor
syntax updates
* Updates CI workflow's caching strategy to use
`actions/setup-node@v3`'s built-in support for yarn berry friendly
caching instead of implementing our own with `actions/cache@v3`
* Updates license-check-and-add and prettier configs to exclude `.yarn/`
* Updates e2e `Dockerfile` to account for new changes
* Drops Lerna dependency in favor of plain Yarn workspaces, similar to
service and action

##### Motivation

* Simplifies some common dev commands. In particular, the command to
update report package snapshots changes from `yarn test:report:e2e -- --
-- -u` to just `yarn test:report:e2e -u`
* Consistency with other repos
* Allows for versioned resolutions (similar to
microsoft/accessibility-insights-action#1596) -
I left modifying existing resolutions out of scope since this PR was
already pretty large without them, will cover them in a different PR
* Removes a dependency (Lerna)

##### Context

Similar PRs in other repos:

* microsoft/accessibility-insights-service#2210
* microsoft/accessibility-insights-action#1559
* microsoft/accessibility-insights-action#1596
* microsoft/axe-sarif-converter#935
* https://github.com/microsoft/accessibility-insights-docs/pull/1577

#### Pull request checklist
<!-- If a checklist item is not applicable to this change, write "n/a"
in the checkbox -->
- [n/a] Addresses an existing issue: #0000
- [x] Ran `yarn fastpass`
- [n/a] Added/updated relevant unit test(s) (and ran `yarn test`)
- [n/a] Verified code coverage for the changes made. Check coverage
report at: `<rootDir>/test-results/unit/coverage`
- [x] PR title *AND* final merge commit title both start with a semantic
tag (`fix:`, `chore:`, `feat(feature-name):`, `refactor:`). See
`CONTRIBUTING.md`.
- [n/a] (UI changes only) Added screenshots/GIFs to description above
- [n/a] (UI changes only) Verified usability with NVDA/JAWS
DaveTryon pushed a commit to microsoft/axe-sarif-converter that referenced this pull request Mar 29, 2023
#### Details

This PR migrates the repo from Yarn v1 to Yarn v3 for dependency
management. The main migration followed the steps in [Yarn's migration
guide](https://yarnpkg.com/getting-started/migration) for a
non-zero-installs configuration, similar to the migrations in
accessibility-insights-service and accessibility-insights-action.

Besides the required/mechanical steps, this PR also includes:

* Updating the resolutions used by this repo to use versioned
resolutions, similar to
microsoft/accessibility-insights-action#1596
* Modifying the `test-resources` generator setup to work as a run script
from the root directory instead of using a separate `package.json` in
`src/test-resources/generator`. This avoided what would have otherwise
required a more complicated setup using either multiple checked in
copies of yarn or a more complicated workspace setup, and slightly
simplifies the steps for using the generator. I updated the README
instructions accordingly.
* The new `!dist/test-resources` entry in the `files` field of
`package.json` ensures that there is no change to the package
as-distributed because of this update.

As part of these changes, I verified manually that `yarn
generate-test-resources` regenerates the current versions of the test
resource files successfully.

##### Motivation

* Consistency with other repos
* Allows use of versioned resolutions (see
microsoft/accessibility-insights-action#1596 for
why we want them)

##### Context

Similar PRs in other repos:

* microsoft/accessibility-insights-service#2210
* microsoft/accessibility-insights-action#1559

#### Pull request checklist
<!-- If a checklist item is not applicable to this change, write "n/a"
in the checkbox -->
- [x] PR title respects [Conventional
Commits](https://www.conventionalcommits.org) (starts with `fix:`,
`feat:`, etc, and is suitable for user-facing release notes)
- [x] PR contains no breaking changes, **OR** description of both PR
**and final merge commit** starts with `BREAKING CHANGE:`
- [n/a] (if applicable) Addresses issue: #0000
- [n/a] Added relevant unit tests for your changes
- [x] Ran `yarn precheckin`
- [n/a] Verified code coverage for the changes made
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants