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

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

Merged
merged 1 commit into from
Jul 31, 2024

Conversation

thomastaylor312
Copy link
Collaborator

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

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

Signed-off-by: Taylor Thomas <taylor@cosmonic.com>
@lann
Copy link
Collaborator

lann commented Jul 12, 2024

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

  • If local config has a backend_type for the registry, use that (override, not default)
  • If the registry metadata (well-known url) has an explicit preferred protocol, use that
  • If the registry metadata has exactly one protocol configured, use that
  • Default to oci

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

  • If local config has exactly one backend configuration for the registry, use that

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

lann commented Jul 12, 2024

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

@thomastaylor312
Copy link
Collaborator Author

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

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

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

@calvinrp calvinrp merged commit 47ad11a into bytecodealliance:main Jul 31, 2024
2 checks passed
@thomastaylor312 thomastaylor312 deleted the fix/backend_type branch July 31, 2024 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants