bytecodealliance / wasm-pkg-tools

Apache License 2.0
48 stars 11 forks source link

fix(common): Fixes issue where we couldn't get a backend type #68

Closed thomastaylor312 closed 2 months ago

thomastaylor312 commented 2 months ago

This fixes an issue where if you have a single backend type set, you still would get a None backend type. This meant that unless you explicitly specified a type, it would always resolve to OCI as used by the client.

This takes a middle ground approach that lets people write less toml. If an explicit type is not specified and only one type of config exists for a registry, then the backend type of that single config will be used instead. For more than one, you must specify a default.

Please note that I renamed this from type to default as that term better expresses the intent of what this config is doing. A user isn't specifying the type of config so much as the default backend config to use here in the (currently) unlikely case you have a registry that talks more than one protocol

lann commented 2 months ago

I'm not sure about this logic change. My intended behavior for resolving backend is:

This PR adds a new case between the first two options:

This means that an implicit "default backend" would override a registry's explicit "preferred protocol", which seems like it could be a problem if a registry wants to switch preferred protocols.

lann commented 2 months ago

Could you say more about the problem scenario this is trying to fix?

thomastaylor312 commented 2 months ago

This is solving for the code in resolve_source. That currently works by (using original terminology here):

  1. Fetching registry metadata unless the backend type is set to "local"
  2. If the registry config has a backend_type set, then it uses that backend
  3. If there is no backend type, use the preferred protocol from the registry metadata
  4. If still none, use OCI

Based on how we have the config set up right now, it implies that theoretically, you could have more than one backend type per registry, so the point here was to be able to indicate that there is a preferred backend to use of the choices available. It also assumes that if you have only one item set, you intend for that config to be used for the registry. Let me restate what I think you are saying should be the order here so we can make sure we're on the same page.

The order of precedence for which backend config to use should be:

  1. User specified config
  2. Well known registry metadata
  3. Default

If this is right and we go with the way it currently exists on main with a config that looks like this:

[namespace_registries]
test = "localhost:1234"

[registry."localhost:1234".warg]
config_file = "/a/path"

When resolve_source is called by the client, it will not use the config because there is no backend type set. Instead it will default to OCI. This is what I ran into when I was putting together the config for tests in cargo component. It was trying to make a call to the registry using the OCI client. To make this work with the current code on main, your config would have to be:

[namespace_registries]
test = "localhost:1234"

[registry."localhost:1234"]
type = "warg"

[registry."localhost:1234".warg]
config_file = "/a/path"

That feels redundant if all I have is a single config block for warg and seems like it would be really simple to miss and then have people wonder why their config isn't loading properly. This is why I had it default if there was only a single backend set. I do have a backend config, and it is warg by default since there is only one. This is the key scenario I am trying to fix here

The other use case I am less set on. I don't know if there will be registries that support both warg and OCI simultaneously, but if we are going to have multiple backends (which I assumed was the goal because we have the function that iterates over their types), then there should be a way to specify which one is the default. You can see an example of this in the tests I added. We would probably also need a function that lets you fetch while overriding the choice of backend.

So, with that said, we have a few options here:

  1. Keep what is in main and document that you need to specify type if you are providing a config
  2. Use this PR
  3. Change how the resolve_source function works to match what we actually want loading preference to be (probably iterating over backends instead and checking which one to use)
calvinrp commented 2 months ago

In the examples and test cases, there is config_file = "/a/path". Is the config_file field used somewhere or just an example property?

thomastaylor312 commented 2 months ago

That is where the warg config file is loaded from. In the test case, it is just a valid path for testing purposes. In a real config file, that would be pointing to a warg config if it was set