Ensure Corepack Usage for npm, pnpm, and yarn Command Execution #10944

kbukum1 · 2024-11-15T23:10:17Z

What are you trying to accomplish?

This PR ensures that corepack is explicitly invoked when running npm, pnpm, and yarn commands. By calling corepack directly, we can guarantee that the correct package manager versions (installed via Corepack) are used for each command. This change helps prevent version mismatches and enhances consistency across different environments.

This change resolves inconsistencies in package manager versions used in different environments and addresses potential issues caused by mismatched versions.

Anything you want to highlight for special attention from reviewers?

The key change here is the addition of corepack to the npm, pnpm, and yarn command execution. This ensures that the commands run with the versions installed by Corepack. If there were multiple approaches to achieve this, I selected this one to make the execution explicit and straightforward.

How will you know you've accomplished your goal?

To confirm these changes, tests were run to verify that each package manager command executes correctly with Corepack and maintains version consistency.
This PR includes updates to run_npm_command, run_pnpm_command, and run_single_yarn_command to prepend corepack to each command, ensuring the correct version usage.
The implementation was verified by running the package manager commands and confirming the expected behavior.

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.

kbukum1 · 2024-11-15T23:51:59Z

npm_and_yarn/lib/dependabot/npm_and_yarn/helpers.rb

+          "corepack install #{name}@#{version} --global --cache-only",
+          fingerprint: "corepack install <name>@<version> --global --cache-only"
+        )
+


moved from PackageManagerHelper into Helpers.

Nishnha · 2024-11-16T15:23:21Z

npm_and_yarn/lib/dependabot/npm_and_yarn/helpers.rb

+      # Setup yarn and run a single yarn command returning stdout/stderr
+      sig { params(command: String, fingerprint: T.nilable(String)).returns(String) }
+      def self.run_yarn_command(command, fingerprint: nil)
+        setup_yarn_berry


we should check to see if we have run the setup command already and skip it if we have

also, since most of the codebase uses #run_single_yarn_command, should we update that method instead?

Thanks for the feedback. I will check if we can run the run_single_yarn_command and make changes accordingly.

I went over the code. I think we are already doing install in file_fetcher phase. So we don't need to re-run setup for that. Just we need to be sure that we are running commands through corepack. And in the following change we are running all command by using corepack. Also in setup_yarn_berry we are running multiple yarn commands with different conditions so I think this method is doing things with conditions and it is calling run_single_yarn_command which is expected to be right call.

https://github.com/dependabot/dependabot-core/pull/10944/files#diff-85ec3865f2fdb48045a2536b37b847156ed3bd3960afdc8bd900ae568de9acd7R295

If you’re suggesting that we configure setup_yarn_berry once and avoid re-running these commands, I can work on that. However, I think it would be better suited to a separate PR, as our priority is first ensuring everything runs smoothly with Corepack.

Nishnha · 2024-11-16T15:30:09Z

The direction of this PR makes sense to me. Do you have any data showing the inconsistent runs? We could use that to verify that this change makes them consistent again

kbukum1 · 2024-11-18T18:12:58Z

The direction of this PR makes sense to me. Do you have any data showing the inconsistent runs? We could use that to verify that this change makes them consistent again.

@Nishnha
I've checked the metrics, and they indicate that we’re always using the default package manager versions in our images. When I manually tried to retrieve the package manager versions within the image using corepack npm -v, corepack pnpm -v, and corepack yarn -v, I found that only the default installed versions are being used rather than the versions installed after detection.

After investigating further, I discovered that once a package manager is installed with corepack, it also needs to be activated. To ensure we’re using the correct installed version, we should activate it and then run commands using corepack #{name} #{command}.

Here are the details for the deployed and reverted changes for further reference: https://github.com/github/dependabot-updates/issues/7325
Dashboard link for metrics: Dependabot Ecosystem Metrics

kbukum1 · 2024-11-18T20:06:22Z

npm_and_yarn/spec/dependabot/npm_and_yarn/update_checker/version_resolver_spec.rb

@@ -79,7 +82,7 @@
    allow(Dependabot::Experiments).to receive(:enabled?)
      .with(:npm_fallback_version_above_v6).and_return(npm_fallback_version_above_v6_enabled)
    allow(Dependabot::Experiments).to receive(:enabled?)
-      .with(:npm_fallback_version_above_v6).and_return(npm_fallback_version_above_v6_enabled)


…rn_using_corepack

fix installing pnpm version issue improve the finding version code

kbukum1 · 2024-11-18T22:49:42Z

npm_and_yarn/lib/dependabot/npm_and_yarn/package_manager.rb

-            raise_if_unsupported!(name, version.to_s)
-
-            install(name, version) if name == PNPMPackageManager::NAME
-          end
        end


We want to install npm, and yarn versions as well. We removed the condition that only install for pnpm when version is guessed.

kbukum1 · 2024-11-18T22:52:42Z

npm_and_yarn/lib/dependabot/npm_and_yarn/package_manager.rb

-          return manager_name.to_s if @lockfiles[manager_name.to_sym]
-        end
-        nil
+        PACKAGE_MANAGER_CLASSES.keys.map(&:to_s).find { |manager_name| @lockfiles[manager_name.to_sym] }


Fix Sorbet Issue:

Dependabot::Sorbet::Runtime::InformationalError Parameter 'name': Expected type T.nilable(String), got type Hash with value {"npm"=>Dependabot::NpmAndY...pmAndYarn::PNPMPackageManager} Caller: /home/dependabot/npm_and_yarn/lib/dependabot/npm_and_yarn/package_manager.rb:221 Definition: /home/dependabot/npm_and_yarn/lib/dependabot/npm_and_yarn/package_manager.rb:282 (Dependabot::NpmAndYarn::PackageManagerHelper#package_manager_by_name)

kbukum1 · 2024-11-18T22:52:48Z

npm_and_yarn/lib/dependabot/npm_and_yarn/package_manager.rb

-        PACKAGE_MANAGER_CLASSES.each_key do |manager_name| # iterates keys in order as defined in the hash
-          return manager_name.to_s if @manifest_package_manager.start_with?("#{manager_name}@")
+        PACKAGE_MANAGER_CLASSES.keys.map(&:to_s).find do |manager_name|
+          @manifest_package_manager.start_with?("#{manager_name}@")
        end


Fix Sorbet Issue:

Dependabot::Sorbet::Runtime::InformationalError Parameter 'name': Expected type T.nilable(String), got type Hash with value {"npm"=>Dependabot::NpmAndY...pmAndYarn::PNPMPackageManager} Caller: /home/dependabot/npm_and_yarn/lib/dependabot/npm_and_yarn/package_manager.rb:221 Definition: /home/dependabot/npm_and_yarn/lib/dependabot/npm_and_yarn/package_manager.rb:282 (Dependabot::NpmAndYarn::PackageManagerHelper#package_manager_by_name)

kbukum1 · 2024-11-18T23:11:11Z

npm_and_yarn/lib/dependabot/npm_and_yarn/package_manager.rb

+          if version
+            raise_if_unsupported!(name, version.to_s)
+            install(name, version)
+          end
        else


This else is going to be removed in clean-up process of enable_corepack_for_npm_and_yarn feature flag after all checks.

broksonic21 · 2024-11-21T17:17:44Z

Flagging this ticket, which is blocking us on a number of our repositories today, and likely is related to this PR: #10982

github-actions bot added the L: javascript label Nov 15, 2024

kbukum1 force-pushed the kamil/fix_installing_problem_for_npm_pnpm_yarn_using_corepack branch from c7e170f to bcb2cf8 Compare November 15, 2024 23:50

kbukum1 commented Nov 15, 2024

View reviewed changes

kbukum1 force-pushed the kamil/fix_installing_problem_for_npm_pnpm_yarn_using_corepack branch from 2bf87a5 to 49db410 Compare November 16, 2024 01:29

Nishnha reviewed Nov 16, 2024

View reviewed changes

fix corepack package manager installation for npm, pnpm and yarn

6da1382

kbukum1 force-pushed the kamil/fix_installing_problem_for_npm_pnpm_yarn_using_corepack branch from 3dc21c9 to 6da1382 Compare November 18, 2024 18:05

kbukum1 added 4 commits November 18, 2024 11:24

added feature flag control

a566c64

update spec

b0d98c3

enable feature flag for specs

adc4f4e

fix spec

7fa48da

kbukum1 commented Nov 18, 2024

View reviewed changes

kbukum1 added 2 commits November 18, 2024 12:08

unnecessary multiline return

7c145ec

fix spec

e502b93

kbukum1 marked this pull request as ready for review November 18, 2024 20:53

kbukum1 requested a review from a team as a code owner November 18, 2024 20:53

abdulapopoola approved these changes Nov 18, 2024

View reviewed changes

Merge branch 'main' into kamil/fix_installing_problem_for_npm_pnpm_ya…

792d47b

…rn_using_corepack

kbukum1 requested a review from Nishnha November 18, 2024 21:01

fix finding package manager name issue

d0ecf1a

fix installing pnpm version issue improve the finding version code

kbukum1 commented Nov 18, 2024

View reviewed changes

kbukum1 added 3 commits November 18, 2024 14:56

revert back the code syntax

fd7da7b

add condition for feature flag is on

ee0faad

fix lint issue

ca3be71

kbukum1 commented Nov 18, 2024

View reviewed changes

kbukum1 merged commit 8f037cf into main Nov 18, 2024
86 checks passed

kbukum1 deleted the kamil/fix_installing_problem_for_npm_pnpm_yarn_using_corepack branch November 18, 2024 23:58

Nishnha mentioned this pull request Nov 21, 2024

Dependabot enforcing engine-strict but with an old npm on Action Runners in last few days #10982

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure Corepack Usage for npm, pnpm, and yarn Command Execution #10944

Ensure Corepack Usage for npm, pnpm, and yarn Command Execution #10944

kbukum1 commented Nov 15, 2024 •

edited

Loading

kbukum1 Nov 15, 2024 •

edited

Loading

Nishnha Nov 16, 2024

Nishnha Nov 16, 2024

kbukum1 Nov 18, 2024

kbukum1 Nov 18, 2024

kbukum1 Nov 18, 2024

Nishnha commented Nov 16, 2024 •

edited

Loading

kbukum1 commented Nov 18, 2024

kbukum1 Nov 18, 2024

kbukum1 Nov 18, 2024

kbukum1 Nov 18, 2024

kbukum1 Nov 18, 2024

kbukum1 Nov 18, 2024

broksonic21 commented Nov 21, 2024

Ensure Corepack Usage for npm, pnpm, and yarn Command Execution #10944

Ensure Corepack Usage for npm, pnpm, and yarn Command Execution #10944

Conversation

kbukum1 commented Nov 15, 2024 • edited Loading

What are you trying to accomplish?

Anything you want to highlight for special attention from reviewers?

How will you know you've accomplished your goal?

Checklist

kbukum1 Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nishnha commented Nov 16, 2024 • edited Loading

kbukum1 commented Nov 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

broksonic21 commented Nov 21, 2024

kbukum1 commented Nov 15, 2024 •

edited

Loading

kbukum1 Nov 15, 2024 •

edited

Loading

Nishnha commented Nov 16, 2024 •

edited

Loading