make sure the standard algorithms are registered#64
Conversation
vbatts
left a comment
There was a problem hiding this comment.
LGTM
I could only guess that some implementations may want to choose to import those explicit packages, but that probably doesn't matter here.
|
Yeah, I was a bit in doubt, but these seem to be the "minimum" set of them, so I thought it made sense to include them automatically. I stumbled on it when I removed the import in one of our utilities, and as a side-effect, things broke in completely unrelated areas 😂 |
|
I just found that moby is using v1.0.0, which does not yet have this change, this is only in master/main: Lines 7 to 17 in b9e02e0 Would that already be enough? (need to try that) Let me push a quick test commit |
|
Ok, they're needed; I'm moving them to the sha.go file |
This makes sure that the expected algorithms are registered when using this package. Currently, the consumer of the package must import these, otherwise it's not registered, resultig in "unsupported digest algorithm" errors. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
ec210b1 to
084376b
Compare
|
We were discussing if SHA512 should be enabled automatically, as it's generally not "recommended" (doesn't add much over SHA256), but it looks to be mentioned in the spec, so we should probably keep it by default; https://github.com/opencontainers/image-spec/blob/083f635f2b04f7f340685a1e0c4393a0feec3679/descriptor.md#registered-algorithms |
|
Thanks, the update looks good. Considering the algorithms are expected to be registered for the image-spec, it makes sense to do them here since they are registered as part of the base package. If we had wanted to separate them out, then putting them in sub-packages (such as with blake) would have been the way to do it. My nit now requires someone else to LGTM it again though, sorry 😉 |
|
Side note: looks like pullaprove configuration is out of sync with the active list of maintainers;
We should remove it in favour of GitHub's |
|
The list is based on the Github team which is not in sync, so not exactly pullapprove's fault. However, we have had issues with it in the past and it doesn't offer us anything that Github doesn't already give us now. |
commit 572498b moved the ioutils.HashData utility to the libnetwork resolvconf package. After removing, we saw some tests in the reference pacakge failing; === Failed === FAIL: reference TestLoad (0.00s) store_test.go:53: failed to parse reference: unsupported digest algorithm === FAIL: reference TestSave (0.00s) store_test.go:82: failed to parse reference: unsupported digest algorithm === FAIL: reference TestAddDeleteGet (0.00s) store_test.go:174: could not parse reference: unsupported digest algorithm === FAIL: reference TestInvalidTags (0.00s) store_test.go:355: assertion failed: error is not nil: unsupported digest algorithm Those failures were because those tests depended on a side-effect of the ioutils package being imported, which (before removal of HashData) imported crypto/sha256, registering that algorithms, which on its turn was used by github.com/opencontainers/go-digest to determnin if a given algorithm must be accepted (see [go-digest#64]). As a workaround, these imports were added. pkg/ioutils is now imported in less places, and should not be depended on for this purpose. Let's remove this workaround; if needed, these imports can be added in a more relevant location. This reverts commit 98caf09. [go-digest#64]: opencontainers/go-digest#64 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This makes sure that the expected algorithms are registered when using this package.
Currently, the consumer of the package must import these, otherwise it's not registered, resultig in "unsupported digest algorithm" errors.
relates to moby/moby#42755 (comment)