Redesign OCI image status: Display the image's exact repository digest #307
Conversation
twelho
added
kind/api-change
pkg/apis/meta/v1alpha1/image.go
Outdated
| } | ||
|
|
||
| // Digest is a getter for the digest field | ||
| func (o *OCIContentID) Digest() string { |
There was a problem hiding this comment.
return digest.Digest? I think that would make sense
There was a problem hiding this comment.
Done in d4495d0 and also for RepoDigest()
| @@ -47,3 +45,89 @@ func (i *OCIImageRef) UnmarshalJSON(b []byte) error { | |||
| *i, err = NewOCIImageRef(str) | |||
| return err | |||
| } | |||
|
|
|||
| func ParseOCIContentID(str string) (*OCIContentID, error) { | |||
There was a problem hiding this comment.
add a short godoc showing sample valid/invalid strings you can give it?
pkg/apis/meta/v1alpha1/image.go
Outdated
| return fmt.Sprintf("oci://%s", o.RepoDigest()) | ||
| } | ||
|
|
||
| return fmt.Sprintf("docker://%s", o.Digest()) |
There was a problem hiding this comment.
maybe make local constants for these?
| } | ||
|
|
||
| // Remove the "docker://" or "oci://" scheme by only caring about the host and path | ||
| return ParseOCIContentID(u.Host + u.Path) |
There was a problem hiding this comment.
if the input is oci://docker.io/library/node@sha256:abc, is the stuff after @ really preserved? have you tested?
There was a problem hiding this comment.
Yes, it's preserved. I tested it will multiple combinations: https://play.golang.org/p/KMFTEOUBVh_W
| // ID defines the source's ID (e.g. the Docker image ID) | ||
| ID string `json:"id"` | ||
| // ID defines the source's content ID (e.g. the canonical OCI path or Docker image ID) | ||
| ID *meta.OCIContentID `json:"id"` |
There was a problem hiding this comment.
Isn't this pretty much required? I can't think of a situation where only the image/kernel size would be reported.
| // ID defines the source's ID (e.g. the Docker image ID) | ||
| ID string `json:"id"` | ||
| // ID defines the source's content ID (e.g. the canonical OCI path or Docker image ID) | ||
| ID *meta.OCIContentID `json:"id"` |
There was a problem hiding this comment.
Same as above, IMO this field is required.
|
Feedback addressed and CI is now green, going ahead and merging |
luxas
removed
the
kind/enhancement
luxas
changed the title
Fixes #298. This PR implements the new OCI image status API representation described in the aforementioned issue complete with parsing and conversion logic.
cc @luxas