openupm / openupm-cli

The OpenUPM-CLI is a command-line interface for maintaining UPM registries.
https://openupm.com
BSD 3-Clause "New" or "Revised" License
244 stars 13 forks source link

feat: multiple upstreams #397

Closed ComradeVanti closed 1 month ago

ComradeVanti commented 1 month ago

This allows users to use any number of upstream registries when resolving packages.

See #349.

ComradeVanti commented 1 month ago

@favoyang I am done with the business side of allowing users to resolve packages from multiple registries. Only two things remain to do here:

favoyang commented 1 month ago

@favoyang I am done with the business side of allowing users to resolve packages from multiple registries. Only two things remain to do here:

  • Add e2e test: Find some stable public third-party registry with a package that depends on a openupm package for testing. Do you know such a registry?

I’m not sure about that. Maybe you can create a local registry with Verdaccio for end-to-end testing, which is what Verdaccio was originally created for.

  • Implement on cli layer: How should this be done? A separate --registries option for when users want to specify multiple registries? Or simply make --registry a multi-value option? This would be a breaking change. What do you think?

I would prefer to make --registry a multi-value option.

ComradeVanti commented 1 month ago

I’m not sure about that. Maybe you can create a local registry with Verdaccio for end-to-end testing, which is what Verdaccio was originally created for.

Oh yeah thats a good idea. I will implement that in general for all e2e tests.

I would prefer to make --registry a multi-value option.

Ok but that would be a breaking change. People might already have a command like this openupm add --registry http://my.registry.com com.my.package. Now This would break since it would now treat com.my.package as a registry url. But i guess we just need to communicate that in the changelog and make it a major version change.

favoyang commented 1 month ago

Ok but that would be a breaking change. People might already have a command like this openupm add --registry http://my.registry.com com.my.package. Now This would break since it would now treat com.my.package as a registry url. But i guess we just need to communicate that in the changelog and make it a major version change.

Adding global parameters before the sub-command should work fine:

openupm --registry REGISTRY_1 REGISTRY_2 add com.example.pkg

I'm not sure how JS Commander handles global parameters when placed after the sub-command. Also considering pkg here is a required argument.

openupm add --registry REGISTRY_1 REGISTRY_2 com.example.pkg
#   add|install [options] <pkg> [otherPkgs...]
ComradeVanti commented 1 month ago

@favoyang Seems like it all works now. I did not write a test because I could not figure out how to setup verdaccio for an e2e test. if you want to give that a go, that would be neat. Anyway i tested it with the registry that is part of my work institution and it worked with no issue.

ComradeVanti commented 1 month ago

@favoyang I also updated the relevant section in the readme. Please take a look if you think it makes sense.

favoyang commented 1 month ago

@favoyang Seems like it all works now. I did not write a test because I could not figure out how to setup verdaccio for an e2e test. if you want to give that a go, that would be neat. Anyway i tested it with the registry that is part of my work institution and it worked with no issue.

If the registry you referenced is stable enough, we can leave it as it is.

For Verdaccio end-to-end (e2e) tests, the easiest way is to use a GitHub Action to launch a Docker service. Then, you can publish a package to it and access it from the localhost IP.

Here's an example: https://github.com/juanpicado/e2e-ci-example-gh-actions/blob/master/.github/workflows/node.js.yml

You can also launch a verdaccio from the code directly, but it can be quite verbose: https://github.com/juanpicado/verdaccio-end-to-end-tests/blob/main/tasks/index.js

favoyang commented 1 month ago

@favoyang I also updated the relevant section in the readme. Please take a look if you think it makes sense.

It's quite clear.