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

[WIP] Re-tooling remotes resolver dependency #17

Closed
wants to merge 13 commits into from

Conversation

juliusl
Copy link

@juliusl juliusl commented Aug 25, 2021

I am trying to decouple dependencies on containerd, to make the oras-go pkg a bit friendlier to take a dependency on. I'm starting on the resolver code since it is in the center of everything. And I plan on working my way towards images next.

For implementing the resolver I am following the oci-spec line by line, hence my notes in http.go (don't want to miss details). For auth, I started out with some basic oauth2 flows so that I can work without having to install docker. I haven't put much thought into anything beyond that just yet.

I have not started debugging this PR, but I'm anxious about getting too far ahead of myself so I wanted to put it up early. I'm pretty happy with the direction it's headed in so far, but I am open to discuss.

@juliusl juliusl marked this pull request as draft August 25, 2021 08:48
@deitch
Copy link
Contributor

deitch commented Aug 25, 2021

I am surprised this is as small as it is; I expected that retooling to be much bigger.

Speaking of which, is this compatible with our oras-go rewrite #8 ? @SteveLasker and @jdolitsky what is happening with getting that in?

@SteveLasker
Copy link
Contributor

@deitch - #8 (comment)

@juliusl
Copy link
Author

juliusl commented Aug 25, 2021

@deitch Yeah my goal is to make this have a lighter touch and become a more extensible base resolver implementation.

So in theory the integration with #8 will should mostly consist of removing wrapper types, and replacing them with a "friend-type" implementation.

@juliusl
Copy link
Author

juliusl commented Aug 26, 2021

@shizhMSFT Today I got resolve/fetch working. Tomorrow I will finish push.

Do you happen to know the status of the discover REST api? I could get that working next and tackle images.Children soon.

@deitch
Copy link
Contributor

deitch commented Aug 26, 2021

@juliusl your call, of course, but it seems a pity to have to do the work twice, in the current implementation and the Copy one.

Do you happen to know the status of the discover REST api?

How would we handle endpoints that do not yet support it?

@deitch deitch mentioned this pull request Aug 26, 2021
@juliusl
Copy link
Author

juliusl commented Aug 26, 2021

@deitch No worries, I have actually done this before for other projects and each time I go through this I learn something new. It also gives me a chance to get more familiar with the state of things.

How would we handle endpoints that do not yet support it?

I think an opt-in approach would probably be best for now. Unless, is there a way to emulate the discover behavior with the set of existing API's? (<-@sajayantony) I'm actually new to this discussion so I'm still ramping up on the details

@deitch
Copy link
Contributor

deitch commented Aug 27, 2021

I think an opt-in approach would probably be best for now.

I would agree. I sort of did something like it once in another language, but it was a narrow use case, so listing things didn't have too bad a performance hit.

Refactoring was mostly organizing the apis to be
easier to consume and seperate concerns.
hardened by returning and handling error from CheckRedirect
Left todo, need to extract it to make it more generic
@SteveLasker
Copy link
Contributor

SteveLasker commented Sep 1, 2021

Related to the registry (opt-in approach), please see: oras-project/artifacts-spec#12
I'm not suggesting we implement the capabilities API, yet. Rather, see:

Now or Later

For the immediate timeframe, the ORAS libraries will likely just query if the referrers/ API exists to proxy the support of the artifacts.manifest.

I'm also hoping this work doesn't diverge from #8 and we can converge them both.

@juliusl
Copy link
Author

juliusl commented Sep 1, 2021

@SteveLasker no worries this will not diverge from #8, when that get's merged I'll spend time to ensure they integrate

@juliusl
Copy link
Author

juliusl commented Sep 1, 2021

@SteveLasker re: capabilities, is it possible to add to that spec something analogous to the GET /v2/ check for the v2 registry api?
For example GET /oras/ (reference: https://github.com/opencontainers/distribution-spec/blob/main/spec.md#determining-support)

Main remotes library does not have containerd references
Allows pkgs to take dependencies on sibling pkgs
For now need a concrete type for content.Writer,
went with passthrough but it's easy to fix and add more later.
@bridgetkromhout
Copy link

Hi, @juliusl - please ensure you sign-off on your commits for the DCO bot. See https://github.com/oras-project/oras-go/pull/17/checks?check_run_id=3489560861 for details on how to fix it. Thanks.

@sajayantony
Copy link
Contributor

I wanted to thank @juliusl for helping prototype this since it make a lot of the conversations about artifacts and discovery API happen.
Also wanted to confirm if #63 will remove the depedency of containerd and we could close this PR as a part of PR pruning?

@SteveLasker
Copy link
Contributor

+1 to pruning.

@shizhMSFT
Copy link
Contributor

Closing this PR as #63 has been resolved.

@shizhMSFT shizhMSFT closed this Jun 29, 2022
@shizhMSFT
Copy link
Contributor

Many thanks to @juliusl for helping investigating the possibilities on top of the containerd.

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.

6 participants