Skip to content

Add a Reference wrapper type to ociref.Reference to handle our "Docker references" logic more cleanly #32

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

Merged
merged 1 commit into from
Mar 23, 2024

Conversation

tianon
Copy link
Member

@tianon tianon commented Mar 21, 2024

This also allows us to add explicit JSON round-tripping which can handle normalizing/denormalizing for us, so our output strings no longer contain docker.io/[library/]. 🎉

@tianon tianon requested a review from yosifkit as a code owner March 21, 2024 21:59
@tianon
Copy link
Member Author

tianon commented Mar 21, 2024

Poor man's codecov.io:

-.../registry/ref.go:18:                ParseRefNormalized         87.5%
+.../registry/ref.go:16:                ParseRef                   83.3%
+.../registry/ref.go:32:                Normalize                  100.0%
+.../registry/ref.go:45:                String                     100.0%
+.../registry/ref.go:54:                MarshalText                0.0%
+.../registry/ref.go:59:                UnmarshalText              0.0%
-.../registry/synthesize-index.go:17:   SynthesizeIndex            74.1%
+.../registry/synthesize-index.go:16:   SynthesizeIndex            74.1%
-.../registry/synthesize-index.go:141:  setRefAnnotation           100.0%
+.../registry/synthesize-index.go:140:  setRefAnnotation           100.0%
-.../registry/synthesize-index.go:151:  normalizeManifestPlatform  84.6%
+.../registry/synthesize-index.go:150:  normalizeManifestPlatform  84.6%
-total:                                 (statements)               74.3%
+total:                                 (statements)               73.7%

(I think the poor scores on MarshalText and UnmarshalText are due to quirks of how Go counts these things, because they're definitely tested 🙃)

Edit: that came from comparing https://github.com/docker-library/meta-scripts/actions/runs/8366831295/job/22907876286#step:5:354 to https://github.com/docker-library/meta-scripts/actions/runs/8382268246/job/22955713965?pr=32#step:5:354, caveman style

@tianon
Copy link
Member Author

tianon commented Mar 21, 2024

image

The ~5% drop in ParseRefNormalized vs ParseRef is because Normalize's logic used to be in there, but now isn't, so the error handling case is now a larger percentage of the overall function body. 🙃

@tianon
Copy link
Member Author

tianon commented Mar 21, 2024

(I think the poor scores on MarshalText and UnmarshalText are due to quirks of how Go counts these things, because they're definitely tested 🙃)

Ok, something is definitely wrong with these - they really aren't being invoked, and I'm not actually sure why! 😭

@tianon tianon marked this pull request as draft March 21, 2024 22:23
…ocker references" logic more cleanly

This also allows us to add explicit JSON round-tripping which can handle normalizing/denormalizing for us, so our output strings no longer contain `docker.io/[library/]`. 🎉
@tianon
Copy link
Member Author

tianon commented Mar 21, 2024

Ok, managed to find both why it wasn't getting invoked when I thought it should be and updated my test to test it explicitly. 🤘

@tianon
Copy link
Member Author

tianon commented Mar 21, 2024

Much better:

@@ -16,12 +16,16 @@
 .../registry/cache.go:92:              GetBlob                    100.0%
 .../registry/cache.go:96:              GetManifest                100.0%
 .../registry/cache.go:100:             GetTag                     69.6%
-.../registry/client.go:17:             Client                     53.1%
+.../registry/client.go:17:             Client                     55.1%
-.../registry/client.go:135:            EntryForRegistry           72.7%
+.../registry/client.go:135:            EntryForRegistry           81.8%
 .../registry/rate-limits.go:23:        Do                         50.0%
 .../registry/read-helpers.go:15:       readJSONHelper             60.9%
-.../registry/ref.go:18:                ParseRefNormalized         87.5%
+.../registry/ref.go:16:                ParseRef                   83.3%
+.../registry/ref.go:32:                Normalize                  100.0%
+.../registry/ref.go:45:                String                     100.0%
+.../registry/ref.go:54:                MarshalText                100.0%
+.../registry/ref.go:59:                UnmarshalText              100.0%
-.../registry/synthesize-index.go:17:   SynthesizeIndex            74.1%
+.../registry/synthesize-index.go:16:   SynthesizeIndex            77.6%
-.../registry/synthesize-index.go:141:  setRefAnnotation           100.0%
+.../registry/synthesize-index.go:140:  setRefAnnotation           100.0%
-.../registry/synthesize-index.go:151:  normalizeManifestPlatform  84.6%
+.../registry/synthesize-index.go:150:  normalizeManifestPlatform  84.6%
-total:                                 (statements)               74.3%
+total:                                 (statements)               75.6%

https://github.com/docker-library/meta-scripts/actions/runs/8382812520/job/22957329243?pr=32#step:5:426

(I don't know for sure why EntryForRegistry and Client have different coverage here, but I'm guessing we have some quirks on how GitHub is setting up our workers, especially WRT authentication 🙃)

@tianon tianon marked this pull request as ready for review March 21, 2024 23:04
@yosifkit yosifkit merged commit 6e0d121 into docker-library:main Mar 23, 2024
@yosifkit yosifkit deleted the ref-wrapper branch March 23, 2024 00:28
@tianon
Copy link
Member Author

tianon commented Mar 25, 2024

(I don't know for sure why EntryForRegistry and Client have different coverage here, but I'm guessing we have some quirks on how GitHub is setting up our workers, especially WRT authentication 🙃)

I just figured out EntryForRegistry's inconsistency, and I literally have a TODO comment in the code for it!! 🤦

// TODO this will iterate in a random order -- maybe that's fine, but maybe we want something more stable? (the new "SortedKeys" iterator that we might get in go1.23? I guess that was rejected, so "slices.Sorted(maps.Keys)")
for dockerHubHost := range dockerHubHosts {
if dockerHubHost == "" {
continue
}

Because our iteration order is random, sometimes we get coverage on this first continue and sometimes we do not! (and the function is small enough that this actually affects the overall percentage by like 9.1%!)

Thanks Gobama!!

Edit: fix in #36

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.

2 participants