Skip to content

metadata: validation and testing of image store#1583

Merged
stevvooe merged 1 commit intocontainerd:masterfrom
stevvooe:image-storage-validation
Oct 3, 2017
Merged

metadata: validation and testing of image store#1583
stevvooe merged 1 commit intocontainerd:masterfrom
stevvooe:image-storage-validation

Conversation

@stevvooe
Copy link
Copy Markdown
Member

@stevvooe stevvooe commented Oct 3, 2017

Signed-off-by: Stephen J Day [email protected]

@stevvooe stevvooe added this to the 1.0.0 milestone Oct 3, 2017
@codecov-io
Copy link
Copy Markdown

codecov-io commented Oct 3, 2017 •
edited
Loading

Codecov Report

Merging #1583 into master will increase coverage by 3.85%.
The diff coverage is 64.7%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1583      +/-   ##
==========================================
+ Coverage   42.44%   46.29%   +3.85%     
==========================================
  Files          24       24              
  Lines        3362     3378      +16     
==========================================
+ Hits         1427     1564     +137     
+ Misses       1608     1456     -152     
- Partials      327      358      +31
Impacted Files Coverage Δ
metadata/images.go 57.27% <64.7%> (+53.35%) ⬆️
metadata/buckets.go 56% <0%> (+9.33%) ⬆️
metadata/adaptors.go 38.02% <0%> (+16.9%) ⬆️

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 7c9b0ea...698b6d1. Read the comment docs.

dmcgowan
dmcgowan reviewed Oct 3, 2017
View reviewed changes
Comment thread images/image.go Outdated
Target ocispec.Descriptor
// Name of the image.
//
// To be pulled, it must be a reference comapitible with resovlers.
Copy link
Copy Markdown
Member

@dmcgowan dmcgowan Oct 3, 2017

Choose a reason for hiding this comment

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

"resolvers"

@dmcgowan
Copy link
Copy Markdown
Member

dmcgowan commented Oct 3, 2017

One misspelling and LGTM

@stevvooe stevvooe force-pushed the image-storage-validation branch from 4eb1add to 86037be Compare October 3, 2017 01:01
AkihiroSuda
AkihiroSuda reviewed Oct 3, 2017
View reviewed changes
Comment thread images/image.go Outdated
Copy link
Copy Markdown
Member

@AkihiroSuda AkihiroSuda Oct 3, 2017

Choose a reason for hiding this comment

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

typo: comapitible

AkihiroSuda
AkihiroSuda reviewed Oct 3, 2017
View reviewed changes
Comment thread images/image.go Outdated
Copy link
Copy Markdown
Member

@AkihiroSuda AkihiroSuda Oct 3, 2017

Choose a reason for hiding this comment

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

To?

AkihiroSuda
AkihiroSuda reviewed Oct 3, 2017
View reviewed changes
Comment thread images/image.go Outdated
Copy link
Copy Markdown
Member

@AkihiroSuda AkihiroSuda Oct 3, 2017

Choose a reason for hiding this comment

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

Target describes..

@AkihiroSuda
Copy link
Copy Markdown
Member

AkihiroSuda commented Oct 3, 2017

LGTM except typos

@stevvooe stevvooe force-pushed the image-storage-validation branch from 86037be to 3377c61 Compare October 3, 2017 22:51
jessvalarezo
jessvalarezo reviewed Oct 3, 2017
View reviewed changes
Comment thread metadata/images_test.go Outdated
Copy link
Copy Markdown
Contributor

@jessvalarezo jessvalarezo Oct 3, 2017

Choose a reason for hiding this comment

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

when I ran make test after pulling this I got a t.Helper undefined: should that happen?

Copy link
Copy Markdown
Member

@dmcgowan dmcgowan Oct 3, 2017

Choose a reason for hiding this comment

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

@jessvalarezo it was just added in go 1.9

jessvalarezo
jessvalarezo reviewed Oct 3, 2017
View reviewed changes
Comment thread metadata/images_test.go Outdated
Copy link
Copy Markdown
Contributor

@jessvalarezo jessvalarezo Oct 3, 2017

Choose a reason for hiding this comment

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

same here

jessvalarezo
jessvalarezo reviewed Oct 3, 2017
View reviewed changes
Comment thread metadata/images.go Outdated
Copy link
Copy Markdown
Contributor

@jessvalarezo jessvalarezo Oct 3, 2017

Choose a reason for hiding this comment

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

Why are we validating some, not all, of the Target fields?

Copy link
Copy Markdown
Member Author

@stevvooe stevvooe Oct 3, 2017

Choose a reason for hiding this comment

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

These are the ones we store. I've broken these off into another function and made a note of this.

@stevvooe stevvooe force-pushed the image-storage-validation branch from 3377c61 to 698b6d1 Compare October 3, 2017 23:34
@stevvooe
Copy link
Copy Markdown
Member Author

stevvooe commented Oct 3, 2017

@jessvalarezo Updated. PTAL.

jessvalarezo
jessvalarezo reviewed Oct 3, 2017
View reviewed changes
Comment thread metadata/images_test.go Outdated
Copy link
Copy Markdown
Contributor

@jessvalarezo jessvalarezo Oct 3, 2017

Choose a reason for hiding this comment

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

Why do we check the timestamp fieldsUpdatedAt and CreatedAt in addition to checkImagesEqual

Copy link
Copy Markdown
Member Author

@stevvooe stevvooe Oct 3, 2017

Choose a reason for hiding this comment

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

@jessvalarezo The checkXXXTimestamps ensures temporal properties, such as updatedat after createdat and equal just does a straight equality check.

Copy link
Copy Markdown
Contributor

@jessvalarezo jessvalarezo Oct 3, 2017

Choose a reason for hiding this comment

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

oh nvm misread

jessvalarezo
jessvalarezo reviewed Oct 3, 2017
View reviewed changes
Comment thread metadata/images_test.go Outdated
Copy link
Copy Markdown
Contributor

@jessvalarezo jessvalarezo Oct 3, 2017

Choose a reason for hiding this comment

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

cool!

jessvalarezo
jessvalarezo reviewed Oct 3, 2017
View reviewed changes
Comment thread metadata/images.go
return validateTarget(&image.Target)
}

func validateTarget(target *ocispec.Descriptor) error {
Copy link
Copy Markdown
Contributor

@jessvalarezo jessvalarezo Oct 3, 2017

Choose a reason for hiding this comment

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

cool!

@jessvalarezo
Copy link
Copy Markdown
Contributor

jessvalarezo commented Oct 3, 2017

LGTM

@stevvooe stevvooe merged commit f6d8892 into containerd:master Oct 3, 2017
@stevvooe stevvooe deleted the image-storage-validation branch October 3, 2017 23:48
@stevvooe stevvooe mentioned this pull request Oct 30, 2017
Closed
mauriciovasquezbernal pushed a commit to kinvolk/containerd that referenced this pull request Nov 13, 2020
@mikebrow
Merge pull request containerd#1583 from thaJeztah/simplify_ensure_rem…
2e3bebb 
…oveall_windows

pkg/server: make ensureRemoveAll() an alias for os.RemoveAll() on Windows
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dmcgowan dmcgowan dmcgowan left review comments

@AkihiroSuda AkihiroSuda AkihiroSuda left review comments

+1 more reviewer

@jessvalarezo jessvalarezo jessvalarezo left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@stevvooe @codecov-io @dmcgowan @AkihiroSuda @jessvalarezo