added a oci function to support generic image #48
Conversation
Signed-off-by: 7h3-3mp7y-m4n <[email protected]>
|
I know the unit test is missing , I'll do it ASAP ... |
Signed-off-by: 7h3-3mp7y-m4n <[email protected]>
Signed-off-by: 7h3-3mp7y-m4n <[email protected]>
Signed-off-by: 7h3-3mp7y-m4n <[email protected]>
Signed-off-by: 7h3-3mp7y-m4n <[email protected]>
There was a problem hiding this comment.
LGTM on my end, but would love the review from @delca85 to have a second pair of eyes on this one.
Thanks for your contribution, @7h3-3mp7y-m4n!
There was a problem hiding this comment.
Thank you for this work!
I left some comments as I think the implementation might be simplified using the oras-go library.
uriget/example_test.go
Outdated
| fmt.Println("failed to pull OCI image:", err) | ||
| return | ||
| } | ||
| fmt.Println(len(buff) > 0, err) |
There was a problem hiding this comment.
Why do we print err here? It needs to be nil otherwise we would be in the other branch.
uriget/uriget.go
Outdated
| @@ -233,3 +239,38 @@ func (o *options) getGit(ctx context.Context, u *url.URL) ([]byte, error) { | |||
| o.logger.Printf("Read %d bytes from %s", len(buff), filepath.Join(td, subPath)) | |||
| return buff, nil | |||
| } | |||
|
|
|||
| func (o *options) getOci(ctx context.Context, u *url.URL) ([]byte, error) { | |||
| parts := strings.Split(u.Host+u.Path, "/") | |||
There was a problem hiding this comment.
I think we can use ParseReference to parse the artifact url into a Reference.
ref, err := registry.ParseReference(u.Host + u.Path)
if err != nil {
return nil, fmt.Errorf("can't parse artifact url in a valid reference: %w", err)
}
registry := ref.Registry
repo := ref.Repository
var tag = "latest"
if ref.Reference != "" {
tag = ref.Reference
}We might get rid of all the parsing stuff from line 244 to line 256.
uriget/uriget.go
Outdated
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create OCI layout store: %w", err) | ||
| } | ||
| repoUrl := fmt.Sprintf("%s/%s", registry, repo) |
uriget/uriget.go
Outdated
| if err != nil { | ||
| return nil, fmt.Errorf("failed to connect to remote repository: %w", err) | ||
| } | ||
| if strings.HasPrefix(repoUrl, "localhost:") || strings.HasPrefix(repoUrl, "127.0.0.1:") { |
There was a problem hiding this comment.
Do we really need this? Genuine question, I am not sure about it, it sounds weird to me that this is not handled by oras.
There was a problem hiding this comment.
I added it cause I was pulling a local image from my docker remote repo, Basically, the terminal kept complaining about the HTTP request so I had to filter it, to avoid the errors and to pull the image, Maybe it was no use , so I'll remove it
There was a problem hiding this comment.
I wasn't able to find anything like that in all the examples from oras library (here some examples) but I'm not an expert here, as said, it's only a gut feeling that this might be not required :).
There was a problem hiding this comment.
And yes you are right :)
Signed-off-by: 7h3-3mp7y-m4n <[email protected]>
|
Wow, really great job on this one, @7h3-3mp7y-m4n, much appreciated! And thank you very much, @delca85, for the detailed review and suggestions, much appreciated too! |
The main purpose of this PR is to fix #45 by adding the OCI feature mentioned here
Description
Added a new case check for oci and implemented a function to retrieve the image.
What does this PR do?
Types of changes
Checklist: