search

opencontainers / go-digest

Common digest package used across the container ecosystem
https://www.opencontainers.org/
Other
185 stars 59 forks source link

make sure the standard algorithms are registered #64

Closed thaJeztah closed 3 years ago

thaJeztah commented 3 years ago

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 https://github.com/moby/moby/pull/42755#issuecomment-901083537

thaJeztah commented 3 years ago

@vbatts @dmcgowan PTAL 🤗

thaJeztah commented 3 years ago

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 😂

thaJeztah commented 3 years ago

I just found that moby is using v1.0.0, which does not yet have this change, this is only in master/main: https://github.com/opencontainers/go-digest/blob/b9e02e015be61903bbee58e3fd349114fa28e0b4/sha.go#L7-L17

Would that already be enough? (need to try that) Discussing with @dmcgowan on slack, and if we still need the imports, it's probably clearer to move it to that file.

Let me push a quick test commit

thaJeztah commented 3 years ago

Ok, they're needed; I'm moving them to the sha.go file

thaJeztah commented 3 years ago

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

dmcgowan commented 3 years ago

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 :wink:

thaJeztah commented 3 years ago

Side note: looks like pullaprove configuration is out of sync with the active list of maintainers;

Screenshot 2021-08-19 at 00 12 38

We should remove it in favour of GitHub's required checks

dmcgowan commented 3 years ago

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.