-
Notifications
You must be signed in to change notification settings - Fork 105
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
Support registry fallback chains #35
Conversation
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.
Left some comments.
We should implement fallback chains for the in-memory registry so we can write more tests. One test I'd like to see is installing, then a registry closer to the front of the chain adds the same package.
Also, I didn't leave comments for the match
statements matching on true/false
, but we should convert those to if expressions as well.
@@ -108,6 +108,10 @@ impl PackageSource for InMemoryRegistrySource { | |||
|
|||
Ok(entry.contents.clone()) | |||
} | |||
|
|||
fn fallback_sources(&self) -> anyhow::Result<Vec<PackageSourceId>> { | |||
todo!("Implement in-memory fallback sources"); |
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.
Can we use anyhow::bail
here? There's no reason to panic since this is already a fallible function
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.
Is there something wrong with a panic here? I'm not sure I understand why a bail would be preferable to a panic since this is code that shouldn't be running anyway. I like how the todo!
here implies that this is something that needs to be implemented rather than some condition that hasn't been met.
I agree with this however I'm going to add it as a new issue. It's something we can always come back to and I'm eager to get this change finally merged. |
This PR adds support for registry fallback chains. See #34
To support sourcing packages from multiple registries each registry can elect a list of other registries which can be looked at as additional sources of packages. The primary use case is to enable us to create registry which contains packages that depend on packages in another registry, for example a private registry with packages depending on the public registry.
This PR includes significant restructuring of the test registry. Previously tests completely ignored the registry listed in the test package manifest files which isn't acceptable to test this sort of use case. Due to this, the tests have been shifted around and manifest files are now respected, even in tests.
As example we can see a new example test package's manifest
Where the manifest for
biff/minimal
containsThese belong to two different registries,
primary-registry
andtertiary-registry
. The tertiary test registry contains the following index/config.json.Tests look at all of these manifests and correctly resolve packages across multiple registries like this. I see this as a significant increase in the robustness of our integration testing. This also works against a real registry using GitHub/hosted api.
This PR:
fallback_registries
field to the index config.json