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

fix(cli): Support PNPM when retrieving package dependency information #2959

Merged
merged 2 commits into from
Jul 5, 2023

Conversation

jamestelfer
Copy link
Contributor

@jamestelfer jamestelfer commented Jun 23, 2023

Related issue

Fixes #2903

Description

pnpm uses a lock file called pnpm-lock.yaml, which can be used as a strong signal about which package manager is in use. There is an upcoming packageManager element in package.json that could also be used, but in practice this is not yet widespread.

Given the presence of this file, the updated logic switches to use pnpm list instead of npm list, and adapts to the updated file structure. I think it is fairly unlikely that a pnpm-lock.yaml file will exist if pnpm is not in use, so I've kept this very simple.

Further, I haven't made DRY efforts between the two methods, as I wasn't certain how useful that would be. I'm very happy to go further if the reviewers would like me to.

Checklist

  • I have updated the PR title to match CDKTF's style guide
  • I have run the linter on my code locally
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation if applicable
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works if applicable
  • New and existing unit tests pass locally with my changes

Notes on tests:

  • no new unit tests are added -- there does not seem to be any tests around the existing functionality
  • test cases run the same locally -- for me main is not clean, but the results between main and this branch are the same.
  • I have tested projects that use yarn and pnpm with this change without issue

I'm happy to adjust the PR given feedback on any aspect.

@hashicorp-cla
Copy link

hashicorp-cla commented Jun 23, 2023

CLA assistant check
All committers have signed the CLA.

@jamestelfer jamestelfer changed the title Support PNPM when retrieving package dependency information fix(cli): Support PNPM when retrieving package dependency information Jun 23, 2023
Use `pnpnm-lock.yaml` as a signal that `pnpm` is the package manager in
use, and use `pnpm list` to discover environmental information in that
case.
@jamestelfer
Copy link
Contributor Author

I have tested this locally against repositories that use yarn and pnpm as their package manager. The debug command returns consistent results in both cases now, there are no error messages emitted, and functionality (synth/deploy) etc works as expected.

The project using pnpm has no issues currently apart from these seemingly non-functional errors, and is happily in production.

It doesn't appear that there is explicit test coverage against this class, so I haven't added tests for the changes. Happy to take feedback on this.

@jamestelfer
Copy link
Contributor Author

I have run tests locally against main and this branch, and they both appear to fail but in the same way. To the best of my knowledge this change doesn't regress current functionality.

@jamestelfer
Copy link
Contributor Author

jamestelfer commented Jun 24, 2023

Note: I am looking to have the CLA signed on Monday, just double-checking everything with legal at work to make sure all the "T's" are crossed etc.

All sorted, CLA signed.

Copy link
Member

@ansgarm ansgarm left a comment

Choose a reason for hiding this comment

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

Thank you for the PR @jamestelfer! Found one small typo, but other than that it's good to go from my point of view 👍

packages/@cdktf/commons/src/debug.ts Outdated Show resolved Hide resolved
Co-authored-by: Ansgar Mertens <mertens.ansgar@gmail.com>
@mutahhir mutahhir added this pull request to the merge queue Jul 5, 2023
Merged via the queue into hashicorp:main with commit 363ca80 Jul 5, 2023
19 of 20 checks passed
@mutahhir mutahhir mentioned this pull request Jul 5, 2023
@jamestelfer jamestelfer deleted the pnpm-error-fix branch July 7, 2023 03:43
@jamestelfer
Copy link
Contributor Author

Thanks @ansgarm for fixing the typo and your review time! Really appreciate getting this small change in!

@github-actions
Copy link
Contributor

github-actions bot commented Aug 7, 2023

I'm going to lock this pull request because it has been closed for 30 days. This helps our maintainers find and focus on the active issues. If you've found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLI: using pnpm causes multiple npm list errors written to CLI output for provider, synth and debug commands
6 participants