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

Add digestset subpackage #55

Merged
merged 10 commits into from
May 11, 2020
Merged

Conversation

dmcgowan
Copy link
Member

Adds digestset from https://github.com/docker/distribution/tree/master/digestset

Originally the digest set was a part of the digest package, but was split out to make the go-digest package smaller when it moved from docker/distribution to here. This package has remained stable and used in situations when a "short" digest needs to be resolved securely and unambiguously. This package is useful alongside go-digest but can be safely broken out into a separate package, without bloating importers who do not intend to use it.

This is helpful to address some docker/distribution vendoring issues for importers who only need this package (such as containerd/containerd).

dmcgowan and others added 10 commits May 10, 2020 15:52
Set represents a unique set of digests which allow for efficient lookup.
Dumping short codes is a function which takes in a digest set.
Any operation involving short codes may be considered secure if the list of digests added to the set is the complete list of referenceable digests.
Contains benchmarks for Add, Lookup, and Dump.

Signed-off-by: Derek McGowan <[email protected]>
To make the definition of supported digests more clear, we have refactored the
digest package to have a special Algorithm type. This represents the digest's
prefix and we associated various supported hash implementations through
function calls.

Signed-off-by: Stephen J Day <[email protected]>
Add mutex protection around set access

Signed-off-by: Derek McGowan <[email protected]>
Correct spelling of words in source code comments.

Signed-off-by: Aaron Lehmann <[email protected]>
1. when lookup an entry which is missing, it should say NotFound.
2. when add duplicated entry, the entries size should be increased.
3. when add entry which has different algorithm, it should be allowed.

Signed-off-by: zhouhaibing089 <[email protected]>
Signed-off-by: Derek McGowan <[email protected]>
@AkihiroSuda
Copy link
Member

LGTM

Copy link
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM(nb)

@thaJeztah
Copy link
Member

Thanks! Looks like after this is merged, we need to do one more vendor round (containerd/cri and back)

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@dmcgowan
Copy link
Member Author

I propose we tag 1.0 after getting this merged in. Then vendoring changes can use the tag.

@Toasterson
Copy link

+1 from me for the 1.0 tag that makes modules behave a little better.

Copy link
Contributor

@stevvooe stevvooe left a comment

Choose a reason for hiding this comment

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

LGTM

This package has a history of being solid. I think the only reason we left it out from the original go-digest project creation was that it was relatively new.

@stevvooe
Copy link
Contributor

Looks like I've been removed from @opencontainers/go-digest-maintainers for some reason, so its not tracking my LGTM.

Copy link
Contributor

@stevvooe stevvooe left a comment

Choose a reason for hiding this comment

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

LGTM

@caniszczyk
Copy link
Contributor

added @stevvooe back and resync'd PullApprove, his next LGTM should be picked up

Copy link
Contributor

@stevvooe stevvooe left a comment

Choose a reason for hiding this comment

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

LGTM

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.

10 participants