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

Import reference #207

Merged
merged 11 commits into from
Sep 5, 2024
Merged

Import reference #207

merged 11 commits into from
Sep 5, 2024

Conversation

cgwalters
Copy link
Contributor

Import Reference type from rust-oci-client

Almost all tools which operate with OCI will end up
wanting to parse and use proper references. Import
the code from the rust-oci-client crate.

I imported the code with history with this blog
which I found helpful:
https://savorywatt.com/2015/01/25/move-files-and-folders-between-git-repos-using-patches/

Changes:

  • Move regex.rs code inline into this file
  • Switch to std LazyLock instead of lazy_static crate

For more see:

Signed-off-by: Colin Walters [email protected]

bacongobbler and others added 11 commits August 30, 2024 08:18
Signed-off-by: Matthew Fisher <[email protected]>
Signed-off-by: Colin Walters <[email protected]>
This commit performs the following changes to the Reference struct:
  * Get rid of the custom implementation of the `Debug` trait, this made
    less obvious the values of the different attributes that are part of
    the struct. We now rely on the derived implementation of the trait,
    which proves to be more useful
  * Replicate the behaviour of the official Docker libraries when
    parsing the Reference from a string.

Prior to this commit the code that parsed the name of a container image
name made some mistakes.

For example, parsing |opensuse/leap:15.3| lead to:
  * Old code: Reference { registry: "opensuse", repository: "leap", tag: Some("15.3"), digest: None }
  * New code: Reference { registry: "docker.io", repository: "opensuse/leap", tag: Some("15.3"), digest: None }

As you can see, the previous code mistakenly thought the image was not
hosted on the Docker Hub.

The wrong behaviour was caused by the code the splits the domain name
from the repository.
This commit changes the old implementation and instead relies on a Rust
port of the official code used by Docker.

As a side effect (which IMHO is a plus), the Reference object is now
populated in a more "verbose" way. The Docker Hub is a "special place",
with official images living in the "library" repository and with the
domain name "docker.io" that can also be omitted.

The parsing code has also been changed to ensure the
`latest` tag is automatically assigned to a Reference when the
input image name doesn't have neither an explicit tag nor a digest.

The unit tests have been updated to reflect these changes. They are also
useful to highlight what is changed by this commit:

DIFF parsing |busybox|
Old code: Reference { registry: "", repository: "busybox", tag: None, digest: None }
New code: Reference { registry: "docker.io", repository: "library/busybox", tag: None, digest: None }

DIFF parsing |test.com:tag|
Old code: Reference { registry: "", repository: "test.com", tag: Some("tag"), digest: None }
New code: Reference { registry: "docker.io", repository: "library/test.com", tag: Some("tag"), digest: None }

DIFF parsing |test.com:5000|
Old code: Reference { registry: "", repository: "test.com", tag: Some("5000"), digest: None }
New code: Reference { registry: "docker.io", repository: "library/test.com", tag: Some("5000"), digest: None }

DIFF parsing |lowercase:Uppercase|
Old code: Reference { registry: "", repository: "lowercase", tag: Some("Uppercase"), digest: None }
New code: Reference { registry: "docker.io", repository: "library/lowercase", tag: Some("Uppercase"), digest: None }

DIFF parsing |foo_bar.com:8080|
Old code: Reference { registry: "", repository: "foo_bar.com", tag: Some("8080"), digest: None }
New code: Reference { registry: "docker.io", repository: "library/foo_bar.com", tag: Some("8080"), digest: None }

DIFF parsing |foo/foo_bar.com:8080|
Old code: Reference { registry: "foo", repository: "foo_bar.com", tag: Some("8080"), digest: None }
New code: Reference { registry: "docker.io", repository: "foo/foo_bar.com", tag: Some("8080"), digest: None }

DIFF parsing |opensuse/leap:15.3|
Old code: Reference { registry: "opensuse", repository: "leap", tag: Some("15.3"), digest: None }
New code: Reference { registry: "docker.io", repository: "opensuse/leap", tag: Some("15.3"), digest: None }

Signed-off-by: Flavio Castelli <[email protected]>
Signed-off-by: Colin Walters <[email protected]>
Address some of the warnings returned by clippy

Signed-off-by: Flavio Castelli <[email protected]>
Signed-off-by: Colin Walters <[email protected]>
Address clippy warnings

Signed-off-by: Flavio Castelli <[email protected]>
Signed-off-by: Colin Walters <[email protected]>
This commit updates the default endpoint for DockerHub to be
`index.docker.io`. This is the default endpoint that the Docker CLI is
using.

Running `docker info`, this is the endpoint returned for the registry:

```
$ docker info | grep Registry
 Registry: https://index.docker.io/v1
```

This means we can now push references to
`index.docker.io/user/repo:tag`.

Signed-off-by: Radu Matei <[email protected]>
Signed-off-by: Colin Walters <[email protected]>
Closes containers#142

Signed-off-by: Taylor Thomas <[email protected]>
Signed-off-by: Colin Walters <[email protected]>
You can pull images via a pull-through proxy, this will change the
resolved registry for the actual request, and the original registry of
the image gets forwarded as a hint for the upstream registry in the "ns"
query parameter.

Signed-off-by: Benjamin Neff <[email protected]>
Signed-off-by: Colin Walters <[email protected]>
This allows to create a copy of the reference but setting a new digest.
This also copies the registry_mirror, so the digest also gets pulled
from the same mirror.

Signed-off-by: Benjamin Neff <[email protected]>
Signed-off-by: Colin Walters <[email protected]>
Don't use a fixed digest algorithm length.

Signed-off-by: Benjamin Neff <[email protected]>
Signed-off-by: Colin Walters <[email protected]>
Almost all tools which operate with OCI will end up
wanting to parse and use proper references. Import
the code from the rust-oci-client crate.

I imported the code with history with this blog
which I found helpful:
https://savorywatt.com/2015/01/25/move-files-and-folders-between-git-repos-using-patches/

Changes:

- Move `regex.rs` code inline into this file
- Switch to std `LazyLock` instead of `lazy_static` crate

For more see:

- containers#205
- oras-project/rust-oci-client#159

Signed-off-by: Colin Walters <[email protected]>
// Digests much always be hex-encoded, ensuring that their hex portion will always be
// size*2
if let Some(digest) = reference.digest() {
match digest.split_once(':') {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could also clearly use 5601907 etc. but probably best to keep code changes minimal until we've deduplicated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep I agree here. We just had this as a string for simplicity, but it would probably be better to have it be a concrete digest type. Definitely going to be easier to wait until this is merge though

@flavio
Copy link
Collaborator

flavio commented Sep 3, 2024

I'm totally in favor of this PR. Once merged I'll update rust-oci-client to consume oci-spec-rs

Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

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

@utam0k PTAL. I just have a non-blocking nit.

Comment on lines +162 to +180
/// Returns the name of the registry.
pub fn registry(&self) -> &str {
&self.registry
}

/// Returns the name of the repository.
pub fn repository(&self) -> &str {
&self.repository
}

/// Returns the object's tag, if present.
pub fn tag(&self) -> Option<&str> {
self.tag.as_deref()
}

/// Returns the object's digest, if present.
pub fn digest(&self) -> Option<&str> {
self.digest.as_deref()
}
Copy link
Member

Choose a reason for hiding this comment

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

I guess we could use getset for those.

Copy link
Collaborator

@thomastaylor312 thomastaylor312 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 pulling this all in! Looking forward to using it downstream in oci-client

@saschagrunert saschagrunert merged commit f628532 into containers:main Sep 5, 2024
14 of 15 checks passed
@cgwalters
Copy link
Contributor Author

Reusing this PR as a communication channel...I think it'd probably make sense to add some folks who maintained/wrote the reference type as reviewers on this repository right? @flavio will you be able to watch for changes on this repo too? Any others to add?

@flavio
Copy link
Collaborator

flavio commented Sep 9, 2024

Fine with me. @thomastaylor312 could be interested too

@cgwalters
Copy link
Contributor Author

@saschagrunert ok to add those two folks to have write/reviewer access here?

On a related topic I notice this repository (unlike others in the containers/ ecosystem) doesn't have branch protection rules set up which we really should have, i.e. require a PR to push to git main etc. Any objections to me also adding those?

@saschagrunert
Copy link
Member

@saschagrunert ok to add those two folks to have write/reviewer access here?

Yes! Having more maintainers would be awesome within this project.

On a related topic I notice this repository (unlike others in the containers/ ecosystem) doesn't have branch protection rules set up which we really should have, i.e. require a PR to push to git main etc. Any objections to me also adding those?

Sure feel free to add it, thanks you for double ckecking!

@cgwalters
Copy link
Contributor Author

I have invited flavio and thomastaylor312 with write role to this repo, and I have added branch protections that:

  • Require a PR before pushing
  • Require a review before a PR can be merged

Which I think is a standard baseline and we now have enough people to match that criteria.

@saschagrunert
Copy link
Member

Thank you @cgwalters! 🙏

@cgwalters
Copy link
Contributor Author

Related to this...as this project has grown some, does anyone have thoughts on possibly submitting an application to the CNCF ? I think it'd be a good home for a project such as this; we're a multi-vendor/organization ASL2.0 project.

(And if we do I'd like to add https://github.com/containers/ocidir-rs/ too)

@saschagrunert
Copy link
Member

@cgwalters would that project fit more specifically into the OCI?

@cgwalters
Copy link
Contributor Author

Seems likely yep

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants