-
Notifications
You must be signed in to change notification settings - Fork 551
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
feat: implement local puller and pusher #1871
Conversation
Can you fix the build errors so we can see if any other tests are affected?
|
Done |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1871 +/- ##
==========================================
- Coverage 71.65% 70.43% -1.22%
==========================================
Files 123 128 +5
Lines 9928 10320 +392
==========================================
+ Hits 7114 7269 +155
- Misses 2115 2310 +195
- Partials 699 741 +42 ☔ View full report in Codecov by Sentry. |
bd592cd
to
cc34721
Compare
pkg/v1/layout/puller.go
Outdated
path Path | ||
} | ||
|
||
func NewPuller(path Path) remote.Puller { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want to define a new interface in v1
that both remote.Puller
and layout.Source
would implement.
In my mind I'd want:
package v1
type Source interface{
// minimal subset of methods needed to read things
}
type Sink interface{
// minimal subset of methods needed to write things
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would remove the dependency between layout
+ remote
and avoid the need to convert remote.Puller
into a struct below. Spitballing the set of methods:
type Source interface {
Layer(...)
Head(...)
Get(...)
Artifact(...)
}
type Sink interface {
Push(...)
Upload(...) // Maybe not?
}
The verbs are a bit clunky for non-remote implementors, but I don't see a clean way around that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, i can't put source and sink into v1 root folder as it creates cycle between v1
-> partial.Artifact
-> v1
Would putting this into its own directory be acceptable?
package github.com/google/go-containerregistry/cmd/crane
imports github.com/google/go-containerregistry/cmd/crane/cmd
imports github.com/google/go-containerregistry/internal/cmd
imports github.com/google/go-containerregistry/internal/verify
imports github.com/google/go-containerregistry/pkg/v1
imports github.com/google/go-containerregistry/pkg/v1/partial
imports github.com/google/go-containerregistry/pkg/v1: import cycle not allowed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay i have created the sinksource (name TBD) directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now ready for another look
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jonjohnsonjr what do you think about v1.Artifact
to get around the import cycle?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that one also sounds fair, though now v1.Artifact would depend on bunch of partial interfaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @jonjohnsonjr @imjasonh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
anything i could do to push this over the edge @jonjohnsonjr? I'd really like to land this, as i have some people who's been waiting for it for a long time.
2d2b853
to
13f2238
Compare
Take two on #1433
Most of the crane cli surface will be able to work with images on oci-layout.