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

Implement index mutation #520

Merged
merged 3 commits into from
Sep 13, 2019
Merged

Conversation

jonjohnsonjr
Copy link
Collaborator

@jonjohnsonjr jonjohnsonjr commented Sep 11, 2019

Add an AppendManifests function that adds entries to the manifests clause of an index.

Fixes #474

@codecov-io
Copy link

codecov-io commented Sep 11, 2019

Codecov Report

Merging #520 into master will increase coverage by 0.08%.
The diff coverage is 70.68%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #520      +/-   ##
==========================================
+ Coverage    73.1%   73.19%   +0.08%     
==========================================
  Files          92       94       +2     
  Lines        3938     4006      +68     
==========================================
+ Hits         2879     2932      +53     
- Misses        696      703       +7     
- Partials      363      371       +8
Impacted Files Coverage Δ
pkg/v1/mutate/mutate.go 73.8% <100%> (+3.42%) ⬆️
pkg/v1/mutate/image.go 67.47% <67.47%> (ø)
pkg/v1/mutate/index.go 75% <75%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0392546...d056be0. Read the comment docs.

@jonjohnsonjr
Copy link
Collaborator Author

An alternative to having two methods is something like:

type Appendable interface {
	RawManifest() ([]byte, error)
	MediaType() (types.MediaType, error)
}

type IndexAddendum struct {
	Add v1.ImageIndex
	v1.Descriptor
}

func AppendManifest(idx v1.ImageIndex, adds ...IndexAddenedum) v1.ImageIndex {
  switch v := i.(type) {
    case v1.ImageIndex:
      // add to indexMap
    case v1.Image:
      // add to imageMap
    case v1.Layer:
      // TODO: how can we do this, actually?
    default:
      // log warning for unexpected addendum
  }
  // add descriptor to manifest
}

We could also do type checking against something that just implements MediaType? It's hard to come up with something reasonable here if an index can reference arbitrary objects.

I suppose we could also append anything that implements:

type Appendable interface {
	Size() (int64, error)
	MediaType() (types.MediaType, error)
        Digest() (v1.Hash, error) 
}

Neither v1.Image nor v1.ImageIndex implement Size, but it would be trivial to add. We could also add a method that returns a v1.Descriptor, which would allow objects to maintain annotations if they needed. This would be useful for e.g. a foreign v1.Layer that knows what URLs it should have.

@@ -75,7 +76,7 @@ func validateChildren(idx v1.ImageIndex) error {
errs = append(errs, fmt.Sprintf("failed to validate image MediaType[%d](%s): %v", i, desc.Digest, err))
}
default:
return fmt.Errorf("todo: validate index Blob()")
logs.Warn.Printf("Unexpected manifest: %s", desc.MediaType)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to return here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need to add a Blob method to v1.ImageIndex if I want to fix that. I can send a PR for that as well, but as it is this will break our tests if I return an error. I'm pretty sure I do want to add Blob to that interface, but I'd like someone to tell me it's a good idea first :)

@jonjohnsonjr jonjohnsonjr merged commit 164ee21 into google:master Sep 13, 2019
@jonjohnsonjr jonjohnsonjr deleted the mutate-index branch September 13, 2019 22:50
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.

Add index mutation methods to pkg/v1/mutate
3 participants