Remove references to schema1 pacakge from storage #4002
Conversation
davidspek
force-pushed
the
remove-schemav1-storage
branch
from
3d76833
to
88b1416
Compare
davidspek
force-pushed
the
remove-schemav1-storage
branch
2 times, most recently
from
8828101
to
5887c8b
Compare
davidspek
force-pushed
the
remove-schemav1-storage
branch
from
5887c8b
to
ef74aa5
Compare
davidspek
force-pushed
the
remove-schemav1-storage
branch
from
629dac0
to
079cd5b
Compare
davidspek
force-pushed
the
remove-schemav1-storage
branch
from
079cd5b
to
0970e7a
Compare
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## main #4002 +/- ##
==========================================
+ Coverage 57.76% 57.79% +0.03%
==========================================
Files 108 106 -2
Lines 10500 10373 -127
==========================================
- Hits 6065 5995 -70
+ Misses 3762 3716 -46
+ Partials 673 662 -11
☔ View full report in Codecov by Sentry. |
davidspek
force-pushed
the
remove-schemav1-storage
branch
2 times, most recently
from
b8f9132
to
33befb9
Compare
| t.Fatal(err) | ||
| } | ||
| options = append([]RegistryOption{EnableDelete, Schema1SigningKey(k), EnableSchema1}, options...) | ||
| options = append(options, EnableDelete) |
There was a problem hiding this comment.
This is now appending to the options slice in the args instead of creating a new slice. If the variadics are passed from an existing slice it will affect the underlying array of the slice.
This is a test function, and an edge case, but should probably be cleaner.
There was a problem hiding this comment.
It's only scoped to each individual test that needs to create a registry object - these options are not used anywhere else so it's ok, imho, but yeah we can change it back.
There was a problem hiding this comment.
I agree in test code it's really a very minor nit :)
I'd approve without it.
| @@ -215,11 +186,21 @@ func TestDeleteManifestIfTagNotFound(t *testing.T) { | |||
| t.Fatalf("failed to make layers: %v", err) | |||
| } | |||
|
|
|||
| digests1 := []digest.Digest{} | |||
There was a problem hiding this comment.
Is this related to schema1 removal? schema1 took layers and schema2 takes manifests?
There was a problem hiding this comment.
the original coode was calling getKeys(randomLayers1) and getKeys(randomLayers2) when passing the params to estutil.MakeSchema1Manifest; we could have kept that helper maybe; this code just makes it explicit because getKeys was removed (I'm assuming as part of the removal of testutil.MakeSchema1Manifest calls, and then never put back leading to this code. But this code is needed to pass the digests down to testutil.MakeSchema2Manifest instead of the schema1 estutil.MakeSchema1Manifest call. Maybe a bit of an oversight but not incorrect. @davidspek do you wanna put back getKeys and then pass getKeys(randomlayers[12]) down to schema2 manifet func
There was a problem hiding this comment.
That comment was more for my understanding than a request for change, happy to go with either style.
There was a problem hiding this comment.
I think we should put that func (getKeys) back either way. This is a great catch even if we arrived at it by tangent 🙃
There was a problem hiding this comment.
Added back the getKeys() function and am passing that to testutil.MakeSchema2Manifest now.
Signed-off-by: David van der Spek <vanderspek.david@gmail.com>
Signed-off-by: David van der Spek <vanderspek.david@gmail.com>
davidspek
force-pushed
the
remove-schemav1-storage
branch
from
8e6910b
to
c7bdaba
Compare
Part of: #3966
This PR is a step towards the elimination of
schema v1support indistribution.This PR removes schema1 package references from the
registry/storagepackage.