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

feat: add lockfile-include-tarball-url option #5054

Merged
merged 5 commits into from Jul 20, 2022

Conversation

MBelniak
Copy link
Contributor

@MBelniak MBelniak commented Jul 18, 2022

In this PR I add lockfile-include-tarball-url option, which when set to true on npm env, makes the installation process of new packages add their resolved tarball URL to pnpm-lock.yaml

I need a way to save a registry from which each package was fetched in pnpm-lock.yaml.
In my company we have 2 registries - one with officially approved packages, the other one mirroring the public npm registry, for development purposes.
I want to keep the information which registry was used for a given package in pnpm-lock.yaml, so that during PR review we know, if there was a package fetched from unofficial registry and it should be first approved.
I see that sometimes the tarball URL is saved, but only if the url is non-standard.
For my case it's really helpful to keep that information for each package.

@MBelniak MBelniak requested a review from zkochan as a code owner July 18, 2022 12:30
@welcome
Copy link

welcome bot commented Jul 18, 2022

💖 Thanks for opening this pull request! 💖
Please be patient and we will get back to you as soon as we can.

@MBelniak
Copy link
Contributor Author

MBelniak commented Jul 18, 2022

I had a trouble running tests using pnpm --filter=@pnpm/plugin-commands-installation test, because I don't know how to setup a mock registry. I kept getting an error:

GET http://localhost:7774/is-negative: Not Found - 404

So if anyone could guide me on this I would be grateful.

@zkochan
Copy link
Member

zkochan commented Jul 18, 2022

I don't know how to setup a mock registry.

that command should run the registry. Try again.

The setting name is not very good. I'd probably call it something like lockfile-include-tarball-url

Add a test and a changeset.

@MBelniak MBelniak changed the title feat: add save-tarball-url feat: add lockfile-include-tarball-url option Jul 19, 2022
@MBelniak
Copy link
Contributor Author

MBelniak commented Jul 19, 2022

I don't know how to setup a mock registry.

that command should run the registry. Try again.

The setting name is not very good. I'd probably call it something like lockfile-include-tarball-url

Add a test and a changeset.

The registry is set up, but there are no is-positive and is-negative packages there, thus 404. I couldn't find a fix, so I will rely on CI tests.

As requested, I added a test and a changeset and I changed option name to the one suggested. New test passed locally. I hope it is OK.
Let me know if there is anything more I should do.

Also, FYI for each git push I got the error below, so I had to push with --no-verify, because this is not related to my change.

/home/work/pnpm/packages/fetcher-base/src/index.ts
  3:1  error  'ssri' should be listed in the project's dependencies. Run 'npm i -S ssri' to add it  import/no-extraneous-dependencies

@zkochan
Copy link
Member

zkochan commented Jul 19, 2022

Also, FYI for each git push I got the error below, so I had to push with --no-verify, because this is not related to my change.

Did you run pnpm install?

@MBelniak
Copy link
Contributor Author

Also, FYI for each git push I got the error below, so I had to push with --no-verify, because this is not related to my change.

Did you run pnpm install?

Yes, I did.

- add save-tarball-url option, which saves resolved tarball URL to
pnpm-lock.yaml during install or add command
…dd test

- option is now named lockfile-include-tarball-url as suggested by
@zkochan
- add test covering new feature
@zkochan zkochan requested a review from a team July 20, 2022 00:29
@zkochan zkochan changed the title feat: add lockfile-include-tarball-url option feat: add lockfile-include-tarball-url option Jul 20, 2022
@MBelniak
Copy link
Contributor Author

Thanks for the last commit :) So, I guess now I just wait for your approval and merge.

@zkochan zkochan merged commit 406656f into pnpm:main Jul 20, 2022
@welcome
Copy link

welcome bot commented Jul 20, 2022

Congrats on merging your first pull request! 🎉🎉🎉

@MBelniak
Copy link
Contributor Author

Wow, I didn't expect it will be so quick :D Thanks a lot!

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.

None yet

2 participants