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: multiple upstreams #397

Merged
merged 14 commits into from
Sep 21, 2024
Merged

feat: multiple upstreams #397

merged 14 commits into from
Sep 21, 2024

Conversation

ComradeVanti
Copy link
Collaborator

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

See #349.

@ComradeVanti ComradeVanti added the enhancement New feature or request label Sep 15, 2024
@ComradeVanti ComradeVanti self-assigned this Sep 15, 2024
@ComradeVanti ComradeVanti force-pushed the multiple-sources branch 2 times, most recently from d500ecd to e49ea68 Compare September 15, 2024 15:30
@ComradeVanti ComradeVanti marked this pull request as ready for review September 15, 2024 15:55
@ComradeVanti
Copy link
Collaborator Author

ComradeVanti commented Sep 15, 2024

@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?
  • 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?

@favoyang
Copy link
Member

@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
Copy link
Collaborator Author

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
Copy link
Member

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 added a commit that referenced this pull request Sep 17, 2024
This change shrinks `cli/index.ts`. Composition root and other global app setup was moved into `index.ts`. Logic for each command was moved into the corresponding cmd file. This is in preparation for overhauling the option logic which in itself is in preparation for #397.
Rename references to "upstream" with "Unity". This is in preparation for adding multiple upstreams.
Refactor logic to init isUnityPackage variable so that it works with an arbitrary amount of registries.
@ComradeVanti
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

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

@ComradeVanti ComradeVanti merged commit 6d61364 into master Sep 21, 2024
9 checks passed
@ComradeVanti ComradeVanti deleted the multiple-sources branch September 21, 2024 15:40
github-actions bot pushed a commit that referenced this pull request Sep 21, 2024
# [4.2.0](4.1.2...4.2.0) (2024-09-21)

### Features

* multiple upstreams ([#397](#397)) ([6d61364](6d61364))
@favoyang
Copy link
Member

@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
Copy link
Member

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

It's quite clear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants