Skip to content

Commit c189bd4

Browse files
code-ashercode-asher
committed
Split out common add/remove code
This way there will be less duplication among storage implementations. Just goes to show how it can be difficult to abstract before actual implementing that abstraction.
1 parent 74ca4f9 commit c189bd4

File tree

10 files changed

+690
-359
lines changed

10 files changed

+690
-359
lines changed

‎api/api_test.go‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,12 +26,12 @@ import (
2626

2727
type fakeStorage struct{}
2828

29-
func (s *fakeStorage) AddExtension(ctx context.Context, vsix []byte) (*storage.Extension, error) {
30-
return nil, errors.New("not implemented")
29+
func (s *fakeStorage) AddExtension(ctx context.Context, manifest *storage.VSIXManifest, vsix []byte) (string, error) {
30+
return "", errors.New("not implemented")
3131
}
3232

33-
func (s *fakeStorage) RemoveExtension(ctx context.Context, id string, all bool) ([]string, error) {
34-
return nil, errors.New("not implemented")
33+
func (s *fakeStorage) RemoveExtension(ctx context.Context, publisher, extension, version string) error {
34+
return errors.New("not implemented")
3535
}
3636

3737
func (s *fakeStorage) FileServer() http.Handler {
@@ -48,6 +48,10 @@ func (s *fakeStorage) Manifest(ctx context.Context, publisher, extension, versio
4848
return nil, errors.New("not implemented")
4949
}
5050

51+
func (s *fakeStorage) Versions(ctx context.Context, publisher, name string) ([]string, error) {
52+
return nil, errors.New("not implemented")
53+
}
54+
5155
func (s *fakeStorage) WalkExtensions(ctx context.Context, fn func(manifest *storage.VSIXManifest, versions []string) error) error {
5256
return errors.New("not implemented")
5357
}

‎cli/add.go‎

Lines changed: 32 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -52,33 +52,55 @@ func add() *cobra.Command {
5252
return err
5353
}
5454

55+
// The manifest is required to know where to place the extension since it
56+
// is unsafe to rely on the file name or URI.
57+
manifest, err := storage.ReadVSIXManifest(vsix)
58+
if err != nil {
59+
return err
60+
}
61+
5562
// Always local storage for now.
56-
store := storage.NewLocalStorage(ctx, extdir, logger)
57-
ext, err := store.AddExtension(ctx, vsix)
63+
store := storage.NewLocalStorage(extdir, logger)
64+
location, err := store.AddExtension(ctx, manifest, vsix)
5865
if err != nil {
5966
return err
6067
}
6168

62-
depCount := len(ext.Dependencies)
69+
deps := []string{}
70+
pack := []string{}
71+
for _, prop := range manifest.Metadata.Properties.Property {
72+
if prop.Value == "" {
73+
continue
74+
}
75+
switch prop.ID {
76+
case storage.DependencyPropertyType:
77+
deps = append(deps, strings.Split(prop.Value, ",")...)
78+
case storage.PackPropertyType:
79+
pack = append(pack, strings.Split(prop.Value, ",")...)
80+
}
81+
}
82+
83+
depCount := len(deps)
84+
id := storage.ExtensionID(manifest)
6385
summary := []string{
64-
fmt.Sprintf("Unpacked %s to %s", ext.ID, ext.Location),
65-
fmt.Sprintf("%s has %s", ext.ID, util.Plural(depCount, "dependency", "dependencies")),
86+
fmt.Sprintf("Unpacked %s to %s", id, location),
87+
fmt.Sprintf("%s has %s", id, util.Plural(depCount, "dependency", "dependencies")),
6688
}
6789

6890
if depCount > 0 {
69-
for _, id := range ext.Dependencies {
91+
for _, id := range deps {
7092
summary = append(summary, fmt.Sprintf(" - %s", id))
7193
}
7294
}
7395

74-
packCount := len(ext.Pack)
96+
packCount := len(pack)
7597
if packCount > 0 {
76-
summary = append(summary, fmt.Sprintf("%s is in a pack with %s", ext.ID, util.Plural(packCount, "other extension", "")))
77-
for _, id := range ext.Pack {
98+
summary = append(summary, fmt.Sprintf("%s is in a pack with %s", id, util.Plural(packCount, "other extension", "")))
99+
for _, id := range pack {
78100
summary = append(summary, fmt.Sprintf(" - %s", id))
79101
}
80102
} else {
81-
summary = append(summary, fmt.Sprintf("%s is not in a pack", ext.ID))
103+
summary = append(summary, fmt.Sprintf("%s is not in a pack", id))
82104
}
83105

84106
_, err = fmt.Fprintln(cmd.OutOrStdout(), strings.Join(summary, "\n"))

‎cli/remove.go‎

Lines changed: 52 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2,16 +2,20 @@ package cli
22

33
import (
44
"context"
5+
"errors"
56
"fmt"
7+
"os"
68
"path/filepath"
79
"strings"
810

911
"github.com/spf13/cobra"
12+
"golang.org/x/xerrors"
1013

1114
"cdr.dev/slog"
1215
"cdr.dev/slog/sloggers/sloghuman"
1316

1417
"github.com/coder/code-marketplace/storage"
18+
"github.com/coder/code-marketplace/util"
1519
)
1620

1721
func remove() *cobra.Command {
@@ -46,23 +50,51 @@ func remove() *cobra.Command {
4650
return err
4751
}
4852

49-
// Always local storage for now.
50-
store := storage.NewLocalStorage(ctx, extdir, logger)
51-
removed, err := store.RemoveExtension(ctx, args[0], all)
53+
id := args[0]
54+
publisher, name, version, err := storage.ParseExtensionID(id)
5255
if err != nil {
5356
return err
5457
}
5558

56-
removedCount := len(removed)
57-
pluralVersions := "versions"
58-
if removedCount == 1 {
59-
pluralVersions = "version"
59+
if version != "" && all {
60+
return xerrors.Errorf("cannot specify both --all and version %s", version)
61+
}
62+
63+
// Always local storage for now.
64+
store := storage.NewLocalStorage(extdir, logger)
65+
66+
allVersions, err := store.Versions(ctx, publisher, name)
67+
if err != nil && !errors.Is(err, os.ErrNotExist) {
68+
return err
69+
}
70+
71+
versionCount := len(allVersions)
72+
if !all && version != "" && !contains(allVersions, version) {
73+
return xerrors.Errorf("%s does not exist", id)
74+
} else if versionCount == 0 {
75+
return xerrors.Errorf("%s.%s has no versions to delete", publisher, name)
76+
} else if version == "" && !all {
77+
return xerrors.Errorf(
78+
"use %s-<version> to target a specific version or pass --all to delete %s of %s",
79+
id,
80+
util.Plural(versionCount, "version", ""),
81+
id,
82+
)
6083
}
61-
summary := []string{
62-
fmt.Sprintf("Removed %d %s", removedCount, pluralVersions),
84+
err = store.RemoveExtension(ctx, publisher, name, version)
85+
if err != nil {
86+
return err
6387
}
64-
for _, id := range removed {
65-
summary = append(summary, fmt.Sprintf(" - %s", id))
88+
89+
summary := []string{}
90+
if all {
91+
removedCount := len(allVersions)
92+
summary = append(summary, fmt.Sprintf("Removed %s", util.Plural(removedCount, "version", "")))
93+
for _, version := range allVersions {
94+
summary = append(summary, fmt.Sprintf(" - %s", version))
95+
}
96+
} else {
97+
summary = append(summary, fmt.Sprintf("Removed %s", version))
6698
}
6799

68100
_, err = fmt.Fprintln(cmd.OutOrStdout(), strings.Join(summary, "\n"))
@@ -76,3 +108,12 @@ func remove() *cobra.Command {
76108

77109
return cmd
78110
}
111+
112+
func contains(a []string, b string) bool {
113+
for _, astr := range a {
114+
if astr == b {
115+
return true
116+
}
117+
}
118+
return false
119+
}

‎cli/remove_test.go‎

Lines changed: 112 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,16 @@ package cli_test
22

33
import (
44
"bytes"
5+
"fmt"
56
"testing"
67

78
"github.com/stretchr/testify/require"
89

910
"github.com/coder/code-marketplace/cli"
11+
"github.com/coder/code-marketplace/testutil"
1012
)
1113

12-
func TestRemove(t *testing.T) {
14+
func TestRemoveHelp(t *testing.T) {
1315
t.Parallel()
1416

1517
cmd := cli.Root()
@@ -23,3 +25,112 @@ func TestRemove(t *testing.T) {
2325
output := buf.String()
2426
require.Contains(t, output, "Remove an extension", "has help")
2527
}
28+
29+
func TestRemove(t *testing.T) {
30+
t.Parallel()
31+
32+
tests := []struct {
33+
// all means to pass --all.
34+
all bool
35+
// error is the expected error.
36+
error string
37+
// extension is the extension to remove. testutil.Extensions[0] will be
38+
// added with versions a, b, and c before each test.
39+
extension testutil.Extension
40+
// name is the name of the test.
41+
name string
42+
// version is the version to remove.
43+
version string
44+
}{
45+
{
46+
name: "RemoveOne",
47+
extension: testutil.Extensions[0],
48+
version: "a",
49+
},
50+
{
51+
name: "All",
52+
extension: testutil.Extensions[0],
53+
all: true,
54+
},
55+
{
56+
name: "MissingTarget",
57+
error: "target a specific version or pass --all",
58+
extension: testutil.Extensions[0],
59+
},
60+
{
61+
name: "MissingTargetNoVersions",
62+
error: "has no versions",
63+
extension: testutil.Extensions[1],
64+
},
65+
{
66+
name: "AllWithVersion",
67+
error: "cannot specify both",
68+
extension: testutil.Extensions[0],
69+
all: true,
70+
version: "a",
71+
},
72+
{
73+
name: "NoVersion",
74+
error: "does not exist",
75+
extension: testutil.Extensions[0],
76+
version: "d",
77+
},
78+
{
79+
name: "NoVersions",
80+
error: "does not exist",
81+
extension: testutil.Extensions[1],
82+
version: "a",
83+
},
84+
{
85+
name: "AllNoVersions",
86+
error: "has no versions",
87+
extension: testutil.Extensions[1],
88+
all: true,
89+
},
90+
}
91+
92+
for _, test := range tests {
93+
test := test
94+
t.Run(test.name, func(t *testing.T) {
95+
t.Parallel()
96+
97+
extdir := t.TempDir()
98+
ext := testutil.Extensions[0]
99+
testutil.AddExtension(t, ext, extdir, "a")
100+
testutil.AddExtension(t, ext, extdir, "b")
101+
testutil.AddExtension(t, ext, extdir, "c")
102+
103+
id := fmt.Sprintf("%s.%s", test.extension.Publisher, test.extension.Name)
104+
if test.version != "" {
105+
id = fmt.Sprintf("%s-%s", id, test.version)
106+
}
107+
108+
cmd := cli.Root()
109+
args := []string{"remove", id, "--extensions-dir", extdir}
110+
if test.all {
111+
args = append(args, "--all")
112+
}
113+
cmd.SetArgs(args)
114+
buf := new(bytes.Buffer)
115+
cmd.SetOutput(buf)
116+
117+
err := cmd.Execute()
118+
output := buf.String()
119+
120+
if test.error != "" {
121+
require.Error(t, err)
122+
require.Regexp(t, test.error, err.Error())
123+
} else {
124+
require.NoError(t, err)
125+
if test.all {
126+
require.Contains(t, output, "Removed 3 versions")
127+
require.Contains(t, output, " - a")
128+
require.Contains(t, output, " - b")
129+
require.Contains(t, output, " - c")
130+
} else {
131+
require.Contains(t, output, fmt.Sprintf("Removed %s", test.version))
132+
}
133+
}
134+
})
135+
}
136+
}

‎cli/server.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ func server() *cobra.Command {
6969
}
7070

7171
// Always local storage for now.
72-
store := storage.NewLocalStorage(ctx, extdir, logger)
72+
store := storage.NewLocalStorage(extdir, logger)
7373

7474
// Always no database for now.
7575
database := &database.NoDB{

‎database/database_test.go‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,12 +20,12 @@ import (
2020

2121
type memoryStorage struct{}
2222

23-
func (s *memoryStorage) AddExtension(ctx context.Context, vsix []byte) (*storage.Extension, error) {
24-
return nil, errors.New("not implemented")
23+
func (s *memoryStorage) AddExtension(ctx context.Context, manifest *storage.VSIXManifest, vsix []byte) (string, error) {
24+
return "", errors.New("not implemented")
2525
}
2626

27-
func (s *memoryStorage) RemoveExtension(ctx context.Context, id string, all bool) ([]string, error) {
28-
return nil, errors.New("not implemented")
27+
func (s *memoryStorage) RemoveExtension(ctx context.Context, publisher, extension, version string) error {
28+
return errors.New("not implemented")
2929
}
3030

3131
func (s *memoryStorage) FileServer() http.Handler {
@@ -34,6 +34,10 @@ func (s *memoryStorage) FileServer() http.Handler {
3434
})
3535
}
3636

37+
func (s *memoryStorage) Versions(ctx context.Context, publisher, name string) ([]string, error) {
38+
return nil, errors.New("not implemented")
39+
}
40+
3741
func (s *memoryStorage) Manifest(ctx context.Context, publisher, extension, version string) (*storage.VSIXManifest, error) {
3842
for _, ext := range testutil.Extensions {
3943
if ext.Publisher == publisher && ext.Name == extension {

0 commit comments

Comments
 (0)