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

[FEATURE] support for registry config in wasmcloud.toml #1067

Merged

Conversation

ahmedtadde
Copy link
Contributor

@ahmedtadde ahmedtadde commented Nov 21, 2023

Feature or Problem

As a user, I would really like it if I didn't have to manually type out my URL to push my actor every time I need to push a new version.

Related Issues

#860

Release Information

wash-cli v0.23.0

Consumer Impact

better DX

Testing

Acceptance or Integration

intergration_reg_config

@ahmedtadde ahmedtadde requested review from a team as code owners November 21, 2023 22:46
Copy link
Contributor

@vados-cosmonic vados-cosmonic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking on such a meaty task @ahmedtadde , this is awesome. This will improve the UX of wash pulland wash push immensely 🚀 .

I found a few minor things and stuff you might want to think about reorganizing, if you wouldn't mind taking a quick look!

crates/wash-cli/src/common/registry_cmd.rs Outdated Show resolved Hide resolved
crates/wash-cli/src/common/registry_cmd.rs Outdated Show resolved Hide resolved
crates/wash-cli/src/common/registry_cmd.rs Outdated Show resolved Hide resolved
crates/wash-cli/src/common/registry_cmd.rs Outdated Show resolved Hide resolved
crates/wash-cli/tests/common/mod.rs Show resolved Hide resolved
crates/wash-cli/src/common/registry_cmd.rs Outdated Show resolved Hide resolved
crates/wash-lib/src/cli/registry.rs Outdated Show resolved Hide resolved
crates/wash-lib/src/parser/mod.rs Outdated Show resolved Hide resolved
crates/wash-cli/tests/wash_reg.rs Outdated Show resolved Hide resolved
@vados-cosmonic
Copy link
Contributor

vados-cosmonic commented Nov 22, 2023

Hey @ahmedtadde there is one conflict (Cargo.lock), but please let me know when this is ready to re-review and I'll review and merge!

crates/control-interface/src/types.rs Show resolved Hide resolved
crates/wash-cli/src/common/registry_cmd.rs Outdated Show resolved Hide resolved
crates/wash-cli/src/common/registry_cmd.rs Outdated Show resolved Hide resolved
crates/wash-lib/src/parser/mod.rs Outdated Show resolved Hide resolved
crates/wash-lib/src/parser/mod.rs Outdated Show resolved Hide resolved
crates/wash-cli/src/common/registry_cmd.rs Outdated Show resolved Hide resolved
crates/wash-cli/src/common/registry_cmd.rs Show resolved Hide resolved
crates/wash-lib/src/cli/registry.rs Outdated Show resolved Hide resolved
crates/wash-lib/src/cli/registry.rs Outdated Show resolved Hide resolved
crates/wash-lib/src/parser/mod.rs Show resolved Hide resolved
@ahmedtadde
Copy link
Contributor Author

hey @connorsmith256

so assuming that a registry url is configured (via WASH_REG_URL or wasmcloud.toml)... how do we deterministically handle

(1) wash reg pull/push wasmcloud.azurecr.io/echo:0.3.8 vs (2) wash reg pull/push echo:0.3.8

That is, how do we distinguish between a full artifact url (1) and a repository path (2) that is supposed to be prepended by a registry url. I am thinking using oci_distribution:Reference::try_from() could work? if it returns an error, then we assume the input value is meant to be just the repository path instead of the full artifact url.

assuming that the above is handled... the current push/pull commands would basically stay the same except that we add an additional optional arg --registry which fallbacks to WASH_REG_URL or project_config.common.registry.url when available. if the registry url fallback values are not available, we return an error to the user.

now, for the credentials, does the credentials file project_config.common.registry.credentials take precedence over the current context's credentials file? or vice versa? either way, we only return an error when the credentials file is specified, exists and parsing it from json into RegistryCredentialMap fails.

@connorsmith256
Copy link
Contributor

@ahmedtadde here's my proposal, though I'd want to hear from others, including you, since this is a breaking change to wash, no matter which approach we take:

  • --registry becomes an optional argument to push/pull, instead of being a positional arg
    • (or --registry-url? -r for shorthand would be nice too)
    • for reference, we use --oci-registry/OCI_REGISTRY for the host
  • in terms of precedence, we should check in this order, which favors explicit user intent over implicit config:
    • flag
    • env var
    • wasmcloud.toml
  • if no registry URL is provided at all, I agree, we should print an error
  • for credentials, the precedence should follow the same order, for consistency. I don't think we need a default registry credsfile. If you think we should have one, I would prefer it to be per-context, instead of global
  • BONUS: the full artifact URL could be inferred, from a combination of the registry URL + name + version. The artifact name and version could be provided the same ways as the registry URL. That would enable wash reg push without any extra details, for the most common workflow, and commands like wash reg push -v 0.3.8-rc.1 to override a specific parameter
    • this could be done in subsequent PR(s)

Signed-off-by: Ahmed <ahmedtadde@gmail.com>
…lution

Signed-off-by: Ahmed <ahmedtadde@gmail.com>
@ahmedtadde ahmedtadde requested a review from a team as a code owner November 29, 2023 03:15
Copy link
Contributor

@connorsmith256 connorsmith256 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay @ahmedtadde, I've had a lot on my plate

LGTM! Just a few optional nits. @vados-cosmonic could you handle getting this merged?

crates/control-interface/src/types.rs Outdated Show resolved Hide resolved
crates/wash-cli/src/common/registry_cmd.rs Show resolved Hide resolved
crates/wash-cli/tests/wash_reg.rs Outdated Show resolved Hide resolved
@vados-cosmonic
Copy link
Contributor

Sure @connorsmith256 ! Hey @ahmedtadde let me know if you want me to merge this PR immediately, or if you want to address those nits -- I'm fine either way as we can always come back and make the small changes later -- thanks again for the awesome contribution!

Signed-off-by: Ahmed <ahmedtadde@gmail.com>
@ahmedtadde
Copy link
Contributor Author

hey @vados-cosmonic , i pushed a commit to address the nits from @connorsmith256. from my end, PR should be good to merge at this point.

@vados-cosmonic vados-cosmonic merged commit 65d2e28 into wasmCloud:main Dec 5, 2023
47 checks passed
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.

None yet

3 participants