-
Notifications
You must be signed in to change notification settings - Fork 18
feat: nupm registry
subcommand
#122
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
base: main
Are you sure you want to change the base?
Conversation
Seems like you're adding a new concept of "registry index" file that replaces the
|
@kubouch thanks for the feedback, do note this is a draft PR so
I could use some feedback here but I see three usecases when dealing with registries that aren't covered yet:
|
I'd say for consistency, if we initialize something, let's call it
|
I think we should prefer environment variables over filesystem items for configuration of nupm, for these reason:
Since we may collect a few more variables, a nested record like this could simply be added to the end of or sourced by one of the existing config files:
|
@Rydwxz are you suggesting registries to be added manually? I think doing programmatic addition/removal of registries could reduce some UX friction as well as allow for some sanitisation of what goes into the record. |
I want to be able to have a fully declarative configuration that uses |
Hi @mkatychev, first of all thank you for making all those changes to accomodate what I asked! The declarative configuration that I was worried about can be achieved without it, so I am worried that I accidentally added too much to the scope of this PR when I mentioned changing to a nested environment variable. I think it's the most sensible/scalable way to organize things, but it might be a breaking change for people who are manually setting those variables right now. |
@Rydwxz I can revert the nested config structure in this PR. |
@Rydwxz I've reverted the requested changes, this should be ready for review once more |
Sorry for the late reply, I'm not available as much as I used to be. I think some of these would be better moved to a separate PR. Also left some misc comments. I would also leave out About the registries, I think having only the index file should be fine. With the A big part of future nupm is virtual environments using overlays, similar to what I sketched in https://github.com/kubouch/nuun. Overlay would define NUPM_HOME and all the stuff derived from it, such as PATH, the registry index file, etc. Here is an example overlay-creation code (very bare-bones). Some things are better left global, like cache -- the point of cache is to reuse stuff between virtual environments. But I think each virtual environment should have its own registry index, and handling them via overlays lets you set it from I hope it's a bit clearer now. Otherwise, seems good, the registry subcommands can be expanded/tweaked as necessary in future PRs. |
Agreed, my mistake on not removing it, realized it was there to largely test private registries.
Would probably be good to get an overlay test at some point. |
typos reworded pretty print name
54754cb
to
4c55581
Compare
@kubouch hopefully this should address most of the issues. |
OK, I think it's good, except a few minor comments. We can merge it with the NUPM_REGISTRIES and refactor the system as we implement the overlays. I originally thought that it would be better to defer implementing overlays until the base features work reliably well, but given how central overlays are to the overall design, I'm thinking that should be the next step to implement. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you could fix the last few comments, we can merge it then.
$env.NUPM_HOME = null | ||
$env.NUPM_TEMP = null | ||
$env.NUPM_CACHE = null | ||
$env.NUPM_REGISTRIES = null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a callout here, null
will give false positives when checking the presence of a field:
> $env.NUPM_HOME = null; ("NUPM_HOME" in $env)
true
@kubouch hopefully I addressed all the changes for this PR, LMK if there's anything I missed. |
This PR introduces the
registry
subcommand for nupm to allow registry management and support multiple registries: