-
Notifications
You must be signed in to change notification settings - Fork 20
Use npm to manage tool dependencies #607
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
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The "Sync Labels" workflow was originally developed with Arduino firmware repositories in mind. Those projects don't have a lot of existing infrastructure and, since they are intended to be very approachable, the impact of adding additional non-firmware files or folders (especially in the root) must be carefully considered. In that context, a self-contained workflow is desirable. However, the situation is different in the inherently more complex and infrastructure rich tooling projects, which are typically consumed only as a binary by users. For this reason, an alternative standardized version of the "Sync Labels" workflow was produced, which utilizes npm to manage its tool dependencies. The code dependencies of this project and the code infrastructure are already managed via npm, which means the switch to this superior version of the workflow can be made without the need to add any additional infrastructure. This provides some significant benefits: - Controlled updates via Dependabot PRs instead of being subject to immediate breakage resulting from a new tool release - Enables automated vulnerability alerts This is especially important for the github-label-sync tool, since it is making irreversible writes to the GitHub repository.
Some of the assets use tools sourced from the npm software registry. Previously, the version of the tools used was not controlled. This was problematic because: - A different version of the tool may be used on the contributor's machine than on the CI runner, resulting in confusing failures. - The project is immediately subject to disruption or breakage resulting from a release of the tool. --- These tools were installed via either of the following methods: `npx <pkg>` This approach has the following behaviors of interest: https://docs.npmjs.com/cli/v8/commands/npx#description > If any requested packages are not present in the local project dependencies, then they are installed to a folder in the npm cache, which is added to the PATH environment variable in the executed process. > Package names provided without a specifier will be matched with whatever version exists in the local project. Package names with a specifier will only be considered a match if they have the exact same name and version as the local dependency. This means that the version used was: 1. Whatever happens to be present in the local cache 2. The latest available version if it is not already present `npm install --global <pkg>` The latest available version of the package is used. --- The new approach is to specify the version of the tools via the standard npm metadata files (package.json + package-lock.json). This approach was chosen over the `npx <pkg>@<version>` alternative for the following reasons: - Enables automated updates via Dependabot PRs - Enables automated vulnerability alerts - Separates dependency management from the asset contents (i.e., no need to mess with the taskfile or workflow on every update) - Matches how we are already managing Python dependencies (pyproject.toml + poetry.lock)
Previously a task named ts:install-deps was used to install the npm-managed dependencies of the project. At the time the task was added, npm was used to manage the action's TypeScript code dependencies as well as TypeScript-specific development tool dependencies. For this reason, it seemed reasonable to use the "ts" prefix on the task name to place it under the TypeScript "namespace" of tasks. In addition to the TypeScript-specific dependencies, npm is now used to manage tool dependencies for general management of the project not specific to TypeScript. For this reason, and because it is now the standardized task name used by the reusable "template" assets, it is better to use the task with the more appropriate "npm" prefix.
MatteoPologruto
approved these changes
Dec 19, 2022
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.
Thanks Per!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a sync/port of a change made in the upstream assets, validated there and already in use in other Arduino Tooling projects: arduino/tooling-project-assets#240
Some of the project infrastructure use tools sourced from the npm software registry.
Previously, the version of the tools used was not controlled. This was problematic because:
An example of the latter is seen here, where a new release of a transitive dependency caused spurious failure of the "Check Markdown" workflow in that repo:
https://github.com/arduino/serial-monitor/runs/8159584293?check_suite_focus=true
These tools were installed via either of the following methods:
npx <pkg>
This approach has the following behaviors of interest:
https://docs.npmjs.com/cli/v8/commands/npx#description
This means that the version used was:
npm install --global <pkg>
The latest available version of the package is used.
The new approach is to specify the version of the tools via the standard npm metadata files (
package.json
+package-lock.json
). This approach was chosen over thenpx <pkg>@<version>
alternative for the following reasons:pyproject.toml
+poetry.lock
)