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

Added pnpm_9 support to dependabot. #10050

Closed
wants to merge 4 commits into from

Conversation

thavaahariharangit
Copy link
Contributor

@thavaahariharangit thavaahariharangit commented Jun 20, 2024

What are you trying to accomplish?

I have added support to pnpm 9 version

Anything you want to highlight for special attention from reviewers?

Added packageManager property to package.json. This will not only enable the support for pnpm 9 but also for the future versions

How will you know you've accomplished your goal?

wrote Junit test with appropriate fixtures. To prove provided solution works as expected

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.

@thavaahariharangit thavaahariharangit requested a review from a team as a code owner June 20, 2024 12:57
@thavaahariharangit thavaahariharangit changed the title New unit test added to prove the pnpm_9 support is already there in dependabot. Added pnpm_9 support to dependabot. Jun 24, 2024
@deivid-rodriguez
Copy link
Contributor

In general (and while I believe packageManageris a good thing), I believe Dependabot should not enforce any style on user's manifests. I believe the underlying issue here is some bug in cases when users do not specify the packageManager field in their package.json file, correct? If that's the case, I think we should instead try to address that bug.

@thavaahariharangit
Copy link
Contributor Author

thavaahariharangit commented Jun 25, 2024

In general (and while I believe packageManageris a good thing), I believe Dependabot should not enforce any style on user's manifests. I believe the underlying issue here is some bug in cases when users do not specify the packageManager field in their package.json file, correct? If that's the case, I think we should instead try to address that bug.

Hi @deivid-rodriguez this is not the bug, I am trying to implement this as a feature (pnpm version 9 support). This will set the pnpm version to package.json based on the pnpm version given in package-lock.json (provided as part of the manifest file).

@deivid-rodriguez
Copy link
Contributor

deivid-rodriguez commented Jun 25, 2024

Reading the latest comments in #9684, it seems like adding packageManager is a workaround to fix that issue. The way I see it, this means enforcing that workaround on everyone, but doesn't explain why the original bug is happening.

Even if this is intentionally changed as a feature (I disagree, but it's Dependabot's team choice), I think it'd be good to dig into #9684 regardless. I can try have a look.

@deivid-rodriguez
Copy link
Contributor

So I looked into it and fixing that bug "just" requires the default PNPM version run by Dependabot to be upgraded. That makes sense, since it explains why Dependabot works for people using packageManager (corepack will automatically download and use PNPM 9). I can create a PR updating PNPM to see what CI thinks.

@deivid-rodriguez
Copy link
Contributor

I created #10073 explaining what's going on in #9684.

@honeyankit
Copy link
Contributor

@thavaahariharangit We can close this PR, since the fix for the PNPM9 is deployed in the below PR:
#10073

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.

4 participants