-
Notifications
You must be signed in to change notification settings - Fork 99
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
feat: implement deleter #470
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -60,7 +60,7 @@ | |
index *ocispec.Index | ||
indexLock sync.Mutex | ||
|
||
storage content.Storage | ||
storage content.DeletableStorage | ||
tagResolver *resolver.Memory | ||
graph *graph.Memory | ||
} | ||
|
@@ -76,6 +76,7 @@ | |
if err != nil { | ||
return nil, fmt.Errorf("failed to resolve absolute path for %s: %w", root, err) | ||
} | ||
|
||
storage, err := NewStorage(rootAbs) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to create storage: %w", err) | ||
|
@@ -192,6 +193,27 @@ | |
return desc, nil | ||
} | ||
|
||
|
||
// Delete removed a target descriptor from index and storage. | ||
func (s *Store) Delete(ctx context.Context, target ocispec.Descriptor) error { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One tricky thing about deleting is that we need to maintain the predecessors relationship as well. See Store.Push and Store.Predecessors.
carabasdaniel marked this conversation as resolved.
Show resolved
Hide resolved
Comment on lines
+197
to
+198
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As mentioned in #470 (comment), the |
||
resolvers := s.tagResolver.Map() | ||
for reference, desc := range resolvers { | ||
if content.Equal(desc, target) { | ||
s.tagResolver.Delete(reference) | ||
} | ||
} | ||
if err := s.graph.RemoveFromIndex(ctx, s.storage, target); err != nil { | ||
return err | ||
} | ||
if s.AutoSaveIndex { | ||
err := s.SaveIndex() | ||
if err != nil { | ||
return err | ||
} | ||
} | ||
return s.storage.Delete(ctx, target) | ||
} | ||
|
||
// Predecessors returns the nodes directly pointing to the current node. | ||
// Predecessors returns nil without error if the node does not exists in the | ||
// store. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1994,3 +1994,114 @@ func equalDescriptorSet(actual []ocispec.Descriptor, expected []ocispec.Descript | |
} | ||
return true | ||
} | ||
|
||
func TestStore_Delete(t *testing.T) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can some/most of this be deduplicated with the memory_test.go TestStoreDelete with similar contents? Ditto in storage_test.go |
||
content := []byte("hello world") | ||
ref := "hello-world:0.0.1" | ||
desc := ocispec.Descriptor{ | ||
MediaType: "test", | ||
Digest: digest.FromBytes(content), | ||
Size: int64(len(content)), | ||
} | ||
|
||
tempDir := t.TempDir() | ||
s, err := New(tempDir) | ||
if err != nil { | ||
t.Fatal("New() error =", err) | ||
} | ||
ctx := context.Background() | ||
|
||
err = s.Push(ctx, desc, bytes.NewReader(content)) | ||
if err != nil { | ||
t.Errorf("Store.Push() error = %v, wantErr %v", err, false) | ||
} | ||
|
||
err = s.Tag(ctx, desc, ref) | ||
if err != nil { | ||
t.Errorf("error tagging descriptor error = %v, wantErr %v", err, false) | ||
} | ||
|
||
resolvedDescr, err := s.Resolve(ctx, ref) | ||
if err != nil { | ||
t.Errorf("error resolving descriptor error = %v, wantErr %v", err, false) | ||
} | ||
|
||
if !reflect.DeepEqual(resolvedDescr, desc) { | ||
t.Errorf("Store.Resolve() = %v, want %v", resolvedDescr, desc) | ||
} | ||
|
||
err = s.Delete(ctx, resolvedDescr) | ||
if err != nil { | ||
t.Errorf("Store.Delete() = %v, wantErr %v", err, true) | ||
} | ||
|
||
_, err = s.Resolve(ctx, ref) | ||
if !errors.Is(err, errdef.ErrNotFound) { | ||
t.Errorf("descriptor should no longer exist in store = %v, wantErr %v", err, errdef.ErrNotFound) | ||
} | ||
} | ||
|
||
func TestStore_DeleteDescriptoMultipleRefs(t *testing.T) { | ||
content := []byte("hello world") | ||
ref1 := "hello-world:0.0.1" | ||
ref2 := "hello-world:0.0.2" | ||
desc := ocispec.Descriptor{ | ||
MediaType: "test", | ||
Digest: digest.FromBytes(content), | ||
Size: int64(len(content)), | ||
} | ||
|
||
tempDir := t.TempDir() | ||
s, err := New(tempDir) | ||
s.AutoSaveIndex = true | ||
if err != nil { | ||
t.Fatal("New() error =", err) | ||
} | ||
ctx := context.Background() | ||
|
||
err = s.Push(ctx, desc, bytes.NewReader(content)) | ||
if err != nil { | ||
t.Errorf("Store.Push() error = %v, wantErr %v", err, false) | ||
} | ||
|
||
if len(s.index.Manifests) != 0 { | ||
t.Errorf("manifest should be empty but has %d elements", len(s.index.Manifests)) | ||
} | ||
|
||
err = s.Tag(ctx, desc, ref1) | ||
if err != nil { | ||
t.Errorf("error tagging descriptor error = %v, wantErr %v", err, false) | ||
} | ||
|
||
err = s.Tag(ctx, desc, ref2) | ||
if err != nil { | ||
t.Errorf("error tagging descriptor error = %v, wantErr %v", err, false) | ||
} | ||
|
||
if len(s.index.Manifests) != 2 { | ||
t.Errorf("manifest should have %d, but has %d", len(s.index.Manifests), 0) | ||
} | ||
|
||
resolvedDescr, err := s.Resolve(ctx, ref1) | ||
if err != nil { | ||
t.Errorf("error resolving descriptor error = %v, wantErr %v", err, false) | ||
} | ||
|
||
if !reflect.DeepEqual(resolvedDescr, desc) { | ||
t.Errorf("Store.Resolve() = %v, want %v", resolvedDescr, desc) | ||
} | ||
|
||
err = s.Delete(ctx, resolvedDescr) | ||
if err != nil { | ||
t.Errorf("Store.Delete() = %v, wantErr %v", err, true) | ||
} | ||
|
||
if len(s.index.Manifests) != 0 { | ||
t.Errorf("manifest should be empty after delete but has %d", len(s.index.Manifests)) | ||
} | ||
|
||
_, err = s.Resolve(ctx, ref2) | ||
if !errors.Is(err, errdef.ErrNotFound) { | ||
t.Errorf("descriptor should no longer exist in store = %v, wantErr %v", err, errdef.ErrNotFound) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s.resolver
ands.graph
are also required to be updated.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I correct in saying that
s.resolver
needs to be updated (possibly byDelete
ing the relevant key in the Store) for all use cases, buts.graph
being updated is only necessary for use cases where Successor/Predecessor functionality will be used?Of course the final implementation here needs to do both, I'm just trying to better understand the behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s.resolver
is for the tagging service, i.e. forResolve()
, asResolve()
should not resolve a reference to a deleted descriptor. Similarly,s.graph
is forPredecessors()
.Since this PR mainly focuses on OCI store, we can keep the changes to the Memory store in another PR.