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

cache docker types too #227

Merged
merged 1 commit into from
Mar 16, 2021
Merged

cache docker types too #227

merged 1 commit into from
Mar 16, 2021

Conversation

deitch
Copy link
Contributor

@deitch deitch commented Feb 22, 2021

Fixes #226

But worth awaiting the result of #225 before merging in to see any impact.

@deitch
Copy link
Contributor Author

deitch commented Mar 12, 2021

This is the last of the 3 (I think) PRs to handle the caching issue, discussed with @jdolitsky. We had discussed whether or not we should be adding the legacy docker types, in addition to the OCI types, to the list of types to be cached.

This PR assumes they should. You had suggested at one point to make the OCI types be the default, and have an option to override the types to be cached.

I am ok with that, in principle, but it creates some unanswered questions:

append vs replace

Should the option append types, or replace them?

I think replace is just simpler, especially if we make the default types available via a function that hybridStore uses. So if I want to replace, I just replace; if I want to append, I call getDefaultTypes() (or whatever) and then just append to the slice, and pass that as a replace.

where to pass the option

The bigger issue is where to pass this option? It only is relevant to newHybridStoreFromIngester, which currently does not have options, although we can add them. But hybridStore (and newHybridStoreFrom*()) are private. newHybridStoreFromIngester is called only by fetchContent (again private), which in turn is called by Pull.

The only exposed place is Pull. We could pass an option there, but that seems distant from what it is dealing with. To have an option to modify how the deep down cache behaves when calling Pull seems a bit disjoint. Then again, we have an option WithContentProvideIngester, so perhaps it is fine?

I see three options:

  1. Just take this as is. It certainly is the simplest option by far.
  2. Create a new func that returns a PullOpt, named something like WithCacheMediaTypes(...MediaType)
  3. Make hybridStore public/exported, give it a creation-time option to modify cached types, and then consumers can use WithContentProvideIngester

My obvious personal preference is just this PR. It is by far the simplest (2-line change). Josh, thoughts?

@jdolitsky
Copy link
Contributor

@deitch - option 2 is still my preference. This enables oras to be fully agnostic to mediatypes

@deitch
Copy link
Contributor Author

deitch commented Mar 15, 2021

You would want the harder one, wouldn't you?

Let's see what we can do.

Signed-off-by: Avi Deitcher <[email protected]>
@deitch
Copy link
Contributor Author

deitch commented Mar 15, 2021

@jdolitsky take a look at it now. See if this works.

Copy link
Contributor

@jdolitsky jdolitsky left a comment

Choose a reason for hiding this comment

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

this looks great

@deitch
Copy link
Contributor Author

deitch commented Mar 16, 2021

I was waiting for you to merge, then realized, wait a sec, I don't need to. I can do it once you approved (which you did) 😃

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.

hybrid store caching misses docker media types but ChildrenHandler expects them
2 participants