diff --git a/distribution/client.go b/distribution/client.go index 29f4f0e..a601888 100644 --- a/distribution/client.go +++ b/distribution/client.go @@ -6,7 +6,6 @@ import ( "io" "net/http" "os" - "strings" "github.com/sirupsen/logrus" @@ -243,51 +242,62 @@ func (c *Client) GetModel(reference string) (types.Model, error) { return model, nil } +type DeleteModelAction struct { + Untagged *string `json:"Untagged,omitempty"` + Deleted *string `json:"Deleted,omitempty"` +} + +type DeleteModelResponse []DeleteModelAction + // DeleteModel deletes a model -func (c *Client) DeleteModel(reference string, force bool) (string, error) { +func (c *Client) DeleteModel(reference string, force bool) (*DeleteModelResponse, error) { mdl, err := c.store.Read(reference) if err != nil { - return "", err + return &DeleteModelResponse{}, err } id, err := mdl.ID() if err != nil { - return "", fmt.Errorf("getting model ID: %w", err) + return &DeleteModelResponse{}, fmt.Errorf("getting model ID: %w", err) } isTag := id != reference + resp := DeleteModelResponse{} + if isTag { c.log.Infoln("Untagging model:", reference) - if err := c.store.RemoveTags([]string{reference}); err != nil { + tags, err := c.store.RemoveTags([]string{reference}) + if err != nil { c.log.Errorln("Failed to untag model:", err, "tag:", reference) - return "", fmt.Errorf("untagging model: %w", err) + return &DeleteModelResponse{}, fmt.Errorf("untagging model: %w", err) } - } - - if len(mdl.Tags()) > 1 { - if isTag { - // TODO: This should return the full matching tag. - return fmt.Sprintf("Untagged: %s\n", reference), nil - } else if !force { - // if the reference is not a tag and there are multiple tags, return an error unless forced - return "", fmt.Errorf( - "unable to delete %q (must be forced) due to multiple tag references: %w", - reference, ErrConflict, - ) + for _, t := range tags { + resp = append(resp, DeleteModelAction{Untagged: &t}) + } + if len(mdl.Tags()) > 1 { + return &resp, nil } } - var untaggedInfo strings.Builder - for _, tag := range mdl.Tags() { - untaggedInfo.WriteString(fmt.Sprintf("Untagged: %s\n", tag)) + if len(mdl.Tags()) > 1 && !force { + // if the reference is not a tag and there are multiple tags, return an error unless forced + return &DeleteModelResponse{}, fmt.Errorf( + "unable to delete %q (must be forced) due to multiple tag references: %w", + reference, ErrConflict, + ) } c.log.Infoln("Deleting model:", id) - if err := c.store.Delete(id); err != nil { + deletedID, tags, err := c.store.Delete(id) + if err != nil { c.log.Errorln("Failed to delete model:", err, "tag:", reference) - return "", fmt.Errorf("deleting model: %w", err) + return &DeleteModelResponse{}, fmt.Errorf("deleting model: %w", err) } c.log.Infoln("Successfully deleted model:", reference) - return untaggedInfo.String(), nil + for _, t := range tags { + resp = append(resp, DeleteModelAction{Untagged: &t}) + } + resp = append(resp, DeleteModelAction{Deleted: &deletedID}) + return &resp, nil } // Tag adds a tag to a model diff --git a/distribution/delete_test.go b/distribution/delete_test.go index 3819b44..05371db 100644 --- a/distribution/delete_test.go +++ b/distribution/delete_test.go @@ -1,6 +1,7 @@ package distribution import ( + "encoding/json" "errors" "os" "slices" @@ -123,7 +124,7 @@ func TestDeleteModel(t *testing.T) { } // Attempt to delete the model and check for expected error - out, err := client.DeleteModel(tc.ref, tc.force) + resp, err := client.DeleteModel(tc.ref, tc.force) if !errors.Is(err, tc.expectedErr) { t.Fatalf("Expected error %v, got: %v", tc.expectedErr, err) } @@ -131,18 +132,25 @@ func TestDeleteModel(t *testing.T) { return } - expectedOut := "" + expectedOut := DeleteModelResponse{} if slices.Contains(tc.tags, tc.ref) { // tc.ref is a tag - expectedOut = "Untagged: " + tc.ref + "\n" + ref := "index.docker.io/library/" + tc.ref + expectedOut = append(expectedOut, DeleteModelAction{Untagged: &ref}) + if !tc.untagOnly { + expectedOut = append(expectedOut, DeleteModelAction{Deleted: &id}) + } } else { // tc.ref is an ID for _, tag := range tc.tags { - expectedOut += "Untagged: " + tag + "\n" + expectedOut = append(expectedOut, DeleteModelAction{Untagged: &tag}) } + expectedOut = append(expectedOut, DeleteModelAction{Deleted: &tc.ref}) } - if expectedOut != out { - t.Fatalf("Expected output %q, got: %q", expectedOut, out) + expectedOutJson, _ := json.Marshal(expectedOut) + respJson, _ := json.Marshal(resp) + if string(expectedOutJson) != string(respJson) { + t.Fatalf("Expected output %s, got: %s", expectedOutJson, respJson) } // Verify model ref unreachable by ref (untagged) diff --git a/internal/store/index.go b/internal/store/index.go index 641a4d5..3d20e8b 100644 --- a/internal/store/index.go +++ b/internal/store/index.go @@ -39,10 +39,10 @@ func (i Index) Tag(reference string, tag string) (Index, error) { return result, nil } -func (i Index) UnTag(tag string) Index { +func (i Index) UnTag(tag string) (name.Tag, Index) { tagRef, err := name.NewTag(tag) if err != nil { - return Index{} + return name.Tag{}, Index{} } result := Index{ @@ -52,7 +52,7 @@ func (i Index) UnTag(tag string) Index { result.Models = append(result.Models, entry.UnTag(tagRef)) } - return result + return tagRef, result } func (i Index) Find(reference string) (IndexEntry, int, bool) { diff --git a/internal/store/index_test.go b/internal/store/index_test.go index 999ee8f..b998ff6 100644 --- a/internal/store/index_test.go +++ b/internal/store/index_test.go @@ -146,12 +146,15 @@ func TestUntag(t *testing.T) { }, }, } - idx = idx.UnTag("other-tag") + tag, idx := idx.UnTag("other-tag") if len(idx.Models) != 2 { t.Fatalf("Expected 2 models, got %d", len(idx.Models)) } if len(idx.Models[0].Tags) != 1 { t.Fatalf("Expected 1 tag, got %d", len(idx.Models[0].Tags)) } + if tag.String() != "other-tag" { + t.Fatalf("Expected tag 'other-tag', got '%s'", tag) + } }) } diff --git a/internal/store/store.go b/internal/store/store.go index 3caa1ad..d9fe797 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -7,7 +7,7 @@ import ( "path/filepath" "github.com/docker/model-distribution/internal/progress" - + "github.com/google/go-containerregistry/pkg/name" v1 "github.com/google/go-containerregistry/pkg/v1" ) @@ -93,14 +93,14 @@ func (s *LocalStore) List() ([]IndexEntry, error) { } // Delete deletes a model by reference -func (s *LocalStore) Delete(ref string) error { +func (s *LocalStore) Delete(ref string) (string, []string, error) { idx, err := s.readIndex() if err != nil { - return fmt.Errorf("reading models file: %w", err) + return "", nil, fmt.Errorf("reading models file: %w", err) } model, _, ok := idx.Find(ref) if !ok { - return ErrModelNotFound + return "", nil, ErrModelNotFound } // Remove manifest file @@ -140,7 +140,7 @@ func (s *LocalStore) Delete(ref string) error { idx = idx.Remove(model.ID) - return s.writeIndex(idx) + return model.ID, model.Tags, s.writeIndex(idx) } // AddTags adds tags to an existing model @@ -160,15 +160,18 @@ func (s *LocalStore) AddTags(ref string, newTags []string) error { } // RemoveTags removes tags from models -func (s *LocalStore) RemoveTags(tags []string) error { +func (s *LocalStore) RemoveTags(tags []string) ([]string, error) { index, err := s.readIndex() if err != nil { - return fmt.Errorf("reading modelss index: %w", err) + return nil, fmt.Errorf("reading modelss index: %w", err) } + var tagRef name.Tag + var tagRefs []string for _, tag := range tags { - index = index.UnTag(tag) + tagRef, index = index.UnTag(tag) + tagRefs = append(tagRefs, tagRef.Name()) } - return s.writeIndex(index) + return tagRefs, s.writeIndex(index) } // Version returns the store version diff --git a/internal/store/store_test.go b/internal/store/store_test.go index b580aef..752e602 100644 --- a/internal/store/store_test.go +++ b/internal/store/store_test.go @@ -151,10 +151,13 @@ func TestStoreAPI(t *testing.T) { // Test RemoveTags t.Run("RemoveTags", func(t *testing.T) { - err := s.RemoveTags([]string{"api-model:api-v1.0"}) + tags, err := s.RemoveTags([]string{"api-model:api-v1.0"}) if err != nil { t.Fatalf("RemoveTags failed: %v", err) } + if tags[0] != "index.docker.io/library/api-model:api-v1.0" { + t.Fatalf("Expected removed tag 'index.docker.io/library/api-model:api-v1.0', got '%s'", tags[0]) + } // Verify tag was removed from list models, err := s.List() @@ -178,7 +181,7 @@ func TestStoreAPI(t *testing.T) { // Test Delete t.Run("Delete", func(t *testing.T) { - err := s.Delete("api-model:latest") + _, _, err := s.Delete("api-model:latest") if err != nil { t.Fatalf("Delete failed: %v", err) } @@ -192,7 +195,7 @@ func TestStoreAPI(t *testing.T) { // Test Delete Non Existent Model t.Run("Delete", func(t *testing.T) { - err := s.Delete("non-existent-model:latest") + _, _, err := s.Delete("non-existent-model:latest") if !errors.Is(err, store.ErrModelNotFound) { t.Fatalf("Expected ErrModelNotFound, got %v", err) } @@ -242,7 +245,7 @@ func TestStoreAPI(t *testing.T) { } // Delete the model - if err := s.Delete("blob-test:latest"); err != nil { + if _, _, err := s.Delete("blob-test:latest"); err != nil { t.Fatalf("Delete failed: %v", err) } @@ -320,7 +323,7 @@ func TestStoreAPI(t *testing.T) { } // Delete the first model - if err := s.Delete("shared-model-1:latest"); err != nil { + if _, _, err := s.Delete("shared-model-1:latest"); err != nil { t.Fatalf("Delete first model failed: %v", err) } @@ -368,7 +371,7 @@ func TestStoreAPI(t *testing.T) { } // Delete the second model - if err := s.Delete("shared-model-2:latest"); err != nil { + if _, _, err := s.Delete("shared-model-2:latest"); err != nil { t.Fatalf("Delete second model failed: %v", err) }