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

default hybrid store on pull does not pass manifest and index through to ingester #225

Closed
deitch opened this issue Feb 22, 2021 · 3 comments · Fixed by #230
Closed

default hybrid store on pull does not pass manifest and index through to ingester #225

deitch opened this issue Feb 22, 2021 · 3 comments · Fixed by #230

Comments

@deitch
Copy link
Contributor

deitch commented Feb 22, 2021

Looking at this code here:

// Writer begins or resumes the active writer identified by desc
func (s *hybridStore) Writer(ctx context.Context, opts ...content.WriterOpt) (content.Writer, error) {
	var wOpts content.WriterOpts
	for _, opt := range opts {
		if err := opt(&wOpts); err != nil {
			return nil, err
		}
	}

	if isAllowedMediaType(wOpts.Desc.MediaType, ocispec.MediaTypeImageManifest, ocispec.MediaTypeImageIndex) || s.ingester == nil {
		return s.cache.Writer(ctx, opts...)
	}
	return s.ingester.Writer(ctx, opts...)
}

If the media type is a manifest or an index, it stores it in the cache - which can be a good thing, especially when the need to walk children comes about - but it doesn't pass it through to the ingester, as s.cache is just a MemoryStore.

Why would we want only to send the config and layers (and whatever other media types there are) through to the ingester? We don't know how it uses it, shouldn't it be up to the ingester to decide?

Strangely, I have had this work, so maybe I am missing something, or maybe it didn't work and I didn't fully realize it?

cc @jdolitsky @shizhMSFT

@deitch
Copy link
Contributor Author

deitch commented Feb 26, 2021

FWIW, if we can agree on this, I am happy to open a PR for this one and pass them through. I already solved it locally. Let me know.

@jdolitsky
Copy link
Contributor

I would be fine with an an additional option such as WithCachedMediaTypes that allows you to supply your own, defaulting to []string{wOpts.Desc.MediaType, ocispec.MediaTypeImageManifest, ocispec.MediaTypeImageIndex}

@deitch
Copy link
Contributor Author

deitch commented Feb 26, 2021

That could solve both. I will provide a PR for both the caching pass through and the mediatypes next week.

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 a pull request may close this issue.

2 participants