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

Dynamic version selector for Npm and Yarn #10510

Merged
merged 19 commits into from
Sep 6, 2024

Conversation

sachin-sandhu
Copy link
Contributor

@sachin-sandhu sachin-sandhu commented Aug 28, 2024

What are you trying to accomplish?

Background: Dependabot relies on ecosystem helpers (package managers) to perform dependencies updates. The package managers (with version) are defined in Dockerfile per ecosystem. For example: NPM and YARN ecosystem that uses NPM, defines the package manager as following in Dockerfile.

ARG NPM_VERSION=9.6.5
RUN corepack install npm@$NPM_VERSION --global

Once the docker container is up, the respective package managers (with mentioned version) are downloaded and installed within container. The per ecosystem file updater uses the package manager to run the updates. i.e.

def self.run_npm_command(command, fingerprint: command)
        SharedHelpers.run_shell_command("corepack npm #{command}", fingerprint: "corepack npm #{fingerprint}")
end

This static version enforcement poses some issues with updates.

a. As we are targeting version enforcement to maximize compatibility and thus dependabot is not always using latest version of package manager. A repo owner can specify a new version of package manager in his code manifest file (i.e. package.json). The compatibility mismatch can create issues while updating the dependencies.

b. A repo project may decide to lock the package manager version due to legacy nature of project. This means that package manager will refuse to update the dependences due to version mismatch.

Returned error response example

Unsupported engine: wanted: {"node":">=16.16.0 <17.0.0 || ^18"} (current: {"node":"v20.16.0","pnpm":"9.4.0"})

Here the node ver 20.16.0 is installed at docker container initial run (static version mentioned in DockerFile). As the manifest file has mentioned an alternate version range (16.16.0 <17.0.0 || ^18), native helper will refuse to update the dependencies due to version mismatch.

Solution: A proposed solution is to install a version of package manager at runtime that is compatible with one mentioned in manifest file. The manifest file can be parsed (i.e. "engines": {"pnpm": "9.1.2"}) and a compatible version matching the spec can be installed at runtime prior to running updates. runtime can be installed using corepack (corepack install pnpm@9.1.2 --global).

The flow description is as following:

File fetching: During file fetching stage, the manifest files in repo are parsed for setting up any prerequisite for file updater stage. Npm and Yarn manifest file (package.json) contains engines tag that specify the package manager (i.e. npm, yarn, pnpm) along with version number (i.e. "2.3.2"). engines is parsed and sent as version for install using corepack command.

SharedHelpers.run_shell_command(
          "corepack install #{name}@#{version} --global --cache-only",
          fingerprint: "corepack install <name>@<version> --global --cache-only"
        ) 

As for now for limited introduction of this feature. We are only allowing versions that match a linear strict form, such as v20.2.3 . Other tag variations include "yarn": ">=1.22", "node": ">=18", "node": "^18", "node":">=16.16.0 <17.0.0 || ^18" which will not be allowed at this time.

Manifest files may also include tag "packageManager" which specify which package manager (i.e. pnpm, yarn) is intended to be used for updates. A scenario might arise where packageManager has version specificity such as "packageManager": "yarn@3.6.3", incase there is a conflict in version specs between engines and packageManager such as "engines": {"yarn": ">=1.22"} and "packageManager": "yarn@3.6.3" , "packageManager" setting will be prioritized.

File updating: Once the runtime engines package manager version is downloaded, the update is run as usual. A matching runtime environment will be available and updates can be run.

Anything you want to highlight for special attention from reviewers?

Please provide your valuable feedback and suggestions.

How will you know you've accomplished your goal?

Checklist

  • I have run the complete test suite to ensure all tests and linters pass.
  • I have thoroughly tested my code changes to ensure they work as expected, including adding additional tests for new functionality.
  • I have written clear and descriptive commit messages.
  • I have provided a detailed description of the changes in the pull request, including the problem it addresses, how it fixes the problem, and any relevant details about the implementation.
  • I have ensured that the code is well-documented and easy to understand.

Copy link
Contributor

@kbukum1 kbukum1 left a comment

Choose a reason for hiding this comment

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

Is it possible to add Sorbet typings if it doesn't cause issues?

@sachin-sandhu sachin-sandhu changed the title DRAFT :: dynamic version selector for Npm and Yarn Dynamic version selector for Npm and Yarn Sep 4, 2024
@sachin-sandhu sachin-sandhu marked this pull request as ready for review September 5, 2024 12:33
@sachin-sandhu sachin-sandhu requested a review from a team as a code owner September 5, 2024 12:33
Copy link
Member

@abdulapopoola abdulapopoola left a comment

Choose a reason for hiding this comment

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

Approving pending comment resolution and signoff by Jamie. Let's do a careful rollout and validation in production to be sure there are no regressions and this works as expected.

@sachin-sandhu sachin-sandhu requested review from JamieMagee and removed request for honeyankit September 6, 2024 14:14
end

# rubocop:disable Metrics/CyclomaticComplexity
# rubocop:disable Metrics/PerceivedComplexity
def setup(name)
Copy link
Member

Choose a reason for hiding this comment

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

can we please do a PR to add Sorbet typing to this file as we agreed some weeks ago (this can be a separate PR, cc: @kbukum1 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abdulapopoola , ill create a separate PR for this. (i added sorbet for new functions already)

# such as "20.8.7", "8.1.2", "8.21.2",
NODE_ENGINE_SUPPORTED_REGEX = /^\d+(?:\.\d+)*$/

sig { params(manifest_json: T.untyped, name: String).returns(T::Hash[Symbol, T.untyped]) }
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can be more specific here

Suggested change
sig { params(manifest_json: T.untyped, name: String).returns(T::Hash[Symbol, T.untyped]) }
sig { params(manifest_json: T::Hash[String, String], name: String).returns(T::Hash[Symbol, T.untyped]) }

Copy link
Contributor

@JamieMagee JamieMagee left a comment

Choose a reason for hiding this comment

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

Approved pending changes

@sachin-sandhu sachin-sandhu force-pushed the ssandhu/dynamic-version-selector-for-npm branch from 0e157ce to 22646bd Compare September 6, 2024 16:58
@sachin-sandhu sachin-sandhu merged commit ae11a0e into main Sep 6, 2024
65 checks passed
@sachin-sandhu sachin-sandhu deleted the ssandhu/dynamic-version-selector-for-npm branch September 6, 2024 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants