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

schema/validator: Punt sha256/sha512 hex-validation to go-digest #696

Merged
merged 1 commit into from
Jun 16, 2017

Conversation

wking
Copy link
Contributor

@wking wking commented Jun 9, 2017

It's covered this since opencontainers/go-digest#34, so we don't need to do it locally. Also, use a more specific check for ignoring unrecognized algorithms, because we don't want to return nil when go-digest notices an invalid encoding, etc.

This fixes issues like this:

-- FAIL: TestDescriptor (0.09s)
  descriptor_test.go:300: test 14: expected validation failure true but got false, err <nil>

which have been affecting recent unit tests on unrelated PRs.

@wking wking force-pushed the go-digest-unsupported branch from c1ce45f to fa09f5b Compare June 9, 2017 21:10
It's covered this since opencontainers/go-digest@4ca13015 (disallow
upper characters (/A-F/) in hex-encoded portion, 2017-05-12, opencontainers#34), so
we don't need to do it locally.  Also, use a more specific check for
ignoring unrecognized algorithms, because we don't want to return nil
when go-digest notices an invalid encoding, etc.

Signed-off-by: W. Trevor King <wking@tremily.us>
@stevvooe
Copy link
Contributor

stevvooe commented Jun 9, 2017

LGTM

Wow, that code was wrong before.

Approved with PullApprove

@vbatts
Copy link
Member

vbatts commented Jun 16, 2017

since go-digest is under the oci org then it seems fine to delegate this check.
LGTM

Approved with PullApprove

@vbatts vbatts merged commit d207df4 into opencontainers:master Jun 16, 2017
@vbatts vbatts mentioned this pull request Jul 5, 2017
@wking wking deleted the go-digest-unsupported branch September 18, 2017 20:15
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.

3 participants