Skip to content
This repository was archived by the owner on Mar 27, 2024. It is now read-only.

Add a "MutableSource" type that allows for basic image modifications. #180

Merged
merged 1 commit into from
Feb 13, 2018

Conversation

dlorenc
Copy link
Contributor

@dlorenc dlorenc commented Jan 25, 2018

This also introduces a "ProxySource" type, which allows for easier customization.

The main logic in "MutableSource" is in the bookkeeping of changes to the image config and manifest.

}

// GetManifest marshals the stored manifest to the byte format.
func (m *MutableSource) GetManifest(instanceDigest *digest.Digest) ([]byte, string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is instanceDigest being used for anything? if not can you remove it as a parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's needed to implement the interface now... I think it's only real purpose is multi-image images, a.k.a manifest lists.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can add it as a GetManifest(_ *digest.Digest) if its not used


// Also add it to the config.
b := bytes.NewReader(content)
gz, err := gzip.NewReader(b)
Copy link
Contributor

Choose a reason for hiding this comment

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

is this always guaranteed to be gzipped? is it possible some other compression was used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's always gzipped. I think I want to enforce that though with some checks, or possibly take the uncompressed layer instead and do the gzip ourselves, for performance.

m.cfg.RootFS.DiffIDs = append(m.cfg.RootFS.DiffIDs, diffID)
history := manifest.Schema2History{
Created: time.Now(),
Author: "dlorenc",
Copy link
Contributor

Choose a reason for hiding this comment

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

probably want a different placeholder author and comment here :)

@dlorenc
Copy link
Contributor Author

dlorenc commented Jan 29, 2018

cc @nkubala this should be ready for review, I had to fix the cache to make it idempotent.

nkubala
nkubala previously approved these changes Jan 29, 2018
Copy link
Contributor

@nkubala nkubala left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the test 👍

Copy link
Contributor

@r2d4 r2d4 left a comment

Choose a reason for hiding this comment

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

Just one comment about embedding the types in ProxySource

}

// GetManifest marshals the stored manifest to the byte format.
func (m *MutableSource) GetManifest(instanceDigest *digest.Digest) ([]byte, string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can add it as a GetManifest(_ *digest.Digest) if its not used


// ProxySource is a type that implements types.ImageSource by proxying all calls to an underlying implementation.
type ProxySource struct {
Ref types.ImageReference
Copy link
Contributor

Choose a reason for hiding this comment

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

You could consider embedding these three types

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Left the one un-embedded.

}

// GetSignatures returns the image's signatures. It may use a remote (= slow) service.
func (p *ProxySource) GetSignatures(ctx context.Context, d *digest.Digest) ([][]byte, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you embed, you can get rid of all the functions you aren't overriding

@dlorenc
Copy link
Contributor Author

dlorenc commented Feb 7, 2018

OK, this should be RFAL.

@dlorenc dlorenc merged commit e57f9f2 into GoogleContainerTools:master Feb 13, 2018
@dlorenc dlorenc deleted the mutable branch February 13, 2018 18:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants