From c6f366e76ffbc5a43709b5b15c3683b2aa0134da Mon Sep 17 00:00:00 2001 From: Mike Landau Date: Thu, 14 Mar 2024 12:52:32 -0700 Subject: [PATCH 1/5] [plugins] Fix github canonical name --- internal/plugin/github.go | 26 ++++++++++++++++++++++++-- internal/plugin/includable.go | 35 +++++++++++++++++++++++++++++++++-- internal/plugin/local.go | 19 +------------------ 3 files changed, 58 insertions(+), 22 deletions(-) diff --git a/internal/plugin/github.go b/internal/plugin/github.go index c2d44cf252a..bf6cc56a111 100644 --- a/internal/plugin/github.go +++ b/internal/plugin/github.go @@ -5,6 +5,8 @@ import ( "io" "net/http" "net/url" + "regexp" + "strings" "go.jetpack.io/devbox/internal/boxcli/usererr" "go.jetpack.io/devbox/internal/cachehash" @@ -12,7 +14,27 @@ import ( ) type githubPlugin struct { - ref flake.Ref + ref flake.Ref + name string +} + +// Github only allows alphanumeric, hyphen, underscore, and period in repo names. +// but we clean up just in case. +var githubNameRegexp = regexp.MustCompile("[^a-zA-Z0-9-_.]+") + +func newGithubPlugin(ref flake.Ref) *githubPlugin { + plugin := &githubPlugin{ref: ref} + // For backward compatibility, we don't strictly require name to be present + // in github plugins. If it's missing, we just use the directory as the name. + name, _ := getPluginNameFromContent(plugin) + if name == "" { + name = strings.ReplaceAll(ref.Dir, "/", "-") + } + plugin.name = githubNameRegexp.ReplaceAllString( + strings.Join([]string{ref.Owner, ref.Repo, name}, "."), + " ", + ) + return plugin } func (p *githubPlugin) Fetch() ([]byte, error) { @@ -20,7 +42,7 @@ func (p *githubPlugin) Fetch() ([]byte, error) { } func (p *githubPlugin) CanonicalName() string { - return p.ref.Owner + "-" + p.ref.Repo + return p.name } func (p *githubPlugin) Hash() string { diff --git a/internal/plugin/includable.go b/internal/plugin/includable.go index b3ef2e43cc9..f18b7259819 100644 --- a/internal/plugin/includable.go +++ b/internal/plugin/includable.go @@ -1,15 +1,17 @@ package plugin import ( + "encoding/json" "fmt" + "go.jetpack.io/devbox/internal/boxcli/usererr" "go.jetpack.io/devbox/nix/flake" ) type Includable interface { CanonicalName() string - Hash() string FileContent(subpath string) ([]byte, error) + Hash() string LockfileKey() string } @@ -22,8 +24,37 @@ func parseIncludable(includableRef, workingDir string) (Includable, error) { case flake.TypePath: return newLocalPlugin(ref, workingDir) case flake.TypeGitHub: - return &githubPlugin{ref: ref}, nil + return newGithubPlugin(ref), nil default: return nil, fmt.Errorf("unsupported ref type %q", ref.Type) } } + +type fetchable interface { + Includable + Fetch() ([]byte, error) +} + +func getPluginNameFromContent(plugin fetchable) (string, error) { + content, err := plugin.Fetch() + if err != nil { + return "", err + } + m := map[string]any{} + if err := json.Unmarshal(content, &m); err != nil { + return "", err + } + name, ok := m["name"].(string) + if !ok || name == "" { + return "", + usererr.New( + "plugin %s is missing a required field 'name'", plugin.LockfileKey()) + } + if !nameRegex.MatchString(name) { + return "", usererr.New( + "plugin %s has an invalid name %q. Name must match %s", + plugin.LockfileKey(), name, nameRegex, + ) + } + return name, nil +} diff --git a/internal/plugin/local.go b/internal/plugin/local.go index 2851c6eca8e..c2fcd483797 100644 --- a/internal/plugin/local.go +++ b/internal/plugin/local.go @@ -1,13 +1,11 @@ package plugin import ( - "encoding/json" "os" "path/filepath" "regexp" "strings" - "go.jetpack.io/devbox/internal/boxcli/usererr" "go.jetpack.io/devbox/internal/cachehash" "go.jetpack.io/devbox/nix/flake" ) @@ -22,25 +20,10 @@ var nameRegex = regexp.MustCompile(`^[a-zA-Z0-9_\- ]+$`) func newLocalPlugin(ref flake.Ref, pluginDir string) (*LocalPlugin, error) { plugin := &LocalPlugin{ref: ref, pluginDir: pluginDir} - content, err := plugin.Fetch() + name, err := getPluginNameFromContent(plugin) if err != nil { return nil, err } - m := map[string]any{} - if err := json.Unmarshal(content, &m); err != nil { - return nil, err - } - name, ok := m["name"].(string) - if !ok || name == "" { - return nil, - usererr.New("plugin %s is missing a required field 'name'", plugin.Path()) - } - if !nameRegex.MatchString(name) { - return nil, usererr.New( - "plugin %s has an invalid name %q. Name must match %s", - plugin.Path(), name, nameRegex, - ) - } plugin.name = name return plugin, nil } From ec470cb6ba8eccb316c4270f7ba18ca3f0b523ff Mon Sep 17 00:00:00 2001 From: Mike Landau Date: Thu, 14 Mar 2024 13:39:04 -0700 Subject: [PATCH 2/5] Fix tests --- internal/plugin/github.go | 3 ++- internal/plugin/github_test.go | 4 ++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/internal/plugin/github.go b/internal/plugin/github.go index bf6cc56a111..0e332f3e0e5 100644 --- a/internal/plugin/github.go +++ b/internal/plugin/github.go @@ -8,6 +8,7 @@ import ( "regexp" "strings" + "github.com/samber/lo" "go.jetpack.io/devbox/internal/boxcli/usererr" "go.jetpack.io/devbox/internal/cachehash" "go.jetpack.io/devbox/nix/flake" @@ -31,7 +32,7 @@ func newGithubPlugin(ref flake.Ref) *githubPlugin { name = strings.ReplaceAll(ref.Dir, "/", "-") } plugin.name = githubNameRegexp.ReplaceAllString( - strings.Join([]string{ref.Owner, ref.Repo, name}, "."), + strings.Join(lo.Compact([]string{ref.Owner, ref.Repo, name}), "."), " ", ) return plugin diff --git a/internal/plugin/github_test.go b/internal/plugin/github_test.go index 206f0612e3c..0d66869b407 100644 --- a/internal/plugin/github_test.go +++ b/internal/plugin/github_test.go @@ -23,6 +23,7 @@ func TestNewGithubPlugin(t *testing.T) { Owner: "jetpack-io", Repo: "devbox-plugins", }, + name: "jetpack-io.devbox-plugins", }, expectedURL: "https://raw.githubusercontent.com/jetpack-io/devbox-plugins/master", }, @@ -36,6 +37,7 @@ func TestNewGithubPlugin(t *testing.T) { Repo: "devbox-plugins", Dir: "mongodb", }, + name: "jetpack-io.devbox-plugins.mongodb", }, expectedURL: "https://raw.githubusercontent.com/jetpack-io/devbox-plugins/master/mongodb", }, @@ -50,6 +52,7 @@ func TestNewGithubPlugin(t *testing.T) { Ref: "my-branch", Dir: "mongodb", }, + name: "jetpack-io.devbox-plugins.mongodb", }, expectedURL: "https://raw.githubusercontent.com/jetpack-io/devbox-plugins/my-branch/mongodb", }, @@ -64,6 +67,7 @@ func TestNewGithubPlugin(t *testing.T) { Ref: "initials/my-branch", Dir: "mongodb", }, + name: "jetpack-io.devbox-plugins.mongodb", }, expectedURL: "https://raw.githubusercontent.com/jetpack-io/devbox-plugins/initials/my-branch/mongodb", }, From 24514d195b259a4e0ef8e707a85db1490abe16df Mon Sep 17 00:00:00 2001 From: Mike Landau Date: Thu, 14 Mar 2024 14:14:45 -0700 Subject: [PATCH 3/5] Requested changes --- internal/plugin/github.go | 10 +++++++--- internal/plugin/includable.go | 11 ++++++++--- internal/plugin/local.go | 3 --- 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/internal/plugin/github.go b/internal/plugin/github.go index 0e332f3e0e5..453f1aeba3b 100644 --- a/internal/plugin/github.go +++ b/internal/plugin/github.go @@ -8,6 +8,7 @@ import ( "regexp" "strings" + "github.com/pkg/errors" "github.com/samber/lo" "go.jetpack.io/devbox/internal/boxcli/usererr" "go.jetpack.io/devbox/internal/cachehash" @@ -23,11 +24,14 @@ type githubPlugin struct { // but we clean up just in case. var githubNameRegexp = regexp.MustCompile("[^a-zA-Z0-9-_.]+") -func newGithubPlugin(ref flake.Ref) *githubPlugin { +func newGithubPlugin(ref flake.Ref) (*githubPlugin, error) { plugin := &githubPlugin{ref: ref} // For backward compatibility, we don't strictly require name to be present // in github plugins. If it's missing, we just use the directory as the name. - name, _ := getPluginNameFromContent(plugin) + name, err := getPluginNameFromContent(plugin) + if err != nil && !errors.Is(err, errNameMissing) { + return nil, err + } if name == "" { name = strings.ReplaceAll(ref.Dir, "/", "-") } @@ -35,7 +39,7 @@ func newGithubPlugin(ref flake.Ref) *githubPlugin { strings.Join(lo.Compact([]string{ref.Owner, ref.Repo, name}), "."), " ", ) - return plugin + return plugin, nil } func (p *githubPlugin) Fetch() ([]byte, error) { diff --git a/internal/plugin/includable.go b/internal/plugin/includable.go index f18b7259819..02e301222d7 100644 --- a/internal/plugin/includable.go +++ b/internal/plugin/includable.go @@ -3,6 +3,7 @@ package plugin import ( "encoding/json" "fmt" + "regexp" "go.jetpack.io/devbox/internal/boxcli/usererr" "go.jetpack.io/devbox/nix/flake" @@ -24,7 +25,7 @@ func parseIncludable(includableRef, workingDir string) (Includable, error) { case flake.TypePath: return newLocalPlugin(ref, workingDir) case flake.TypeGitHub: - return newGithubPlugin(ref), nil + return newGithubPlugin(ref) default: return nil, fmt.Errorf("unsupported ref type %q", ref.Type) } @@ -35,6 +36,11 @@ type fetchable interface { Fetch() ([]byte, error) } +var ( + nameRegex = regexp.MustCompile(`^[a-zA-Z0-9_\- ]+$`) + errNameMissing = usererr.New("'name' is missing") +) + func getPluginNameFromContent(plugin fetchable) (string, error) { content, err := plugin.Fetch() if err != nil { @@ -47,8 +53,7 @@ func getPluginNameFromContent(plugin fetchable) (string, error) { name, ok := m["name"].(string) if !ok || name == "" { return "", - usererr.New( - "plugin %s is missing a required field 'name'", plugin.LockfileKey()) + fmt.Errorf("%w in plugin %s", errNameMissing, plugin.LockfileKey()) } if !nameRegex.MatchString(name) { return "", usererr.New( diff --git a/internal/plugin/local.go b/internal/plugin/local.go index c2fcd483797..91a123dc264 100644 --- a/internal/plugin/local.go +++ b/internal/plugin/local.go @@ -3,7 +3,6 @@ package plugin import ( "os" "path/filepath" - "regexp" "strings" "go.jetpack.io/devbox/internal/cachehash" @@ -16,8 +15,6 @@ type LocalPlugin struct { pluginDir string } -var nameRegex = regexp.MustCompile(`^[a-zA-Z0-9_\- ]+$`) - func newLocalPlugin(ref flake.Ref, pluginDir string) (*LocalPlugin, error) { plugin := &LocalPlugin{ref: ref, pluginDir: pluginDir} name, err := getPluginNameFromContent(plugin) From 8b082ef5a241e3487fe4f2843b2cf13182a26ae4 Mon Sep 17 00:00:00 2001 From: Mike Landau Date: Thu, 14 Mar 2024 14:33:20 -0700 Subject: [PATCH 4/5] Fix test --- internal/plugin/github_test.go | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/internal/plugin/github_test.go b/internal/plugin/github_test.go index 0d66869b407..34bbaa80c09 100644 --- a/internal/plugin/github_test.go +++ b/internal/plugin/github_test.go @@ -1,8 +1,10 @@ package plugin import ( + "strings" "testing" + "github.com/samber/lo" "github.com/stretchr/testify/assert" "go.jetpack.io/devbox/nix/flake" ) @@ -75,7 +77,8 @@ func TestNewGithubPlugin(t *testing.T) { for _, testCase := range testCases { t.Run(testCase.name, func(t *testing.T) { - actual, _ := parseIncludable(testCase.Include, "") + actual, err := newGithubPluginForTest(testCase.Include) + assert.NoError(t, err) assert.Equal(t, &testCase.expected, actual) u, err := testCase.expected.url("") assert.Nil(t, err) @@ -83,3 +86,19 @@ func TestNewGithubPlugin(t *testing.T) { }) } } + +// keep in sync with newGithubPlugin +func newGithubPluginForTest(include string) (*githubPlugin, error) { + ref, err := flake.ParseRef(include) + if err != nil { + return nil, err + } + + plugin := &githubPlugin{ref: ref} + name := strings.ReplaceAll(ref.Dir, "/", "-") + plugin.name = githubNameRegexp.ReplaceAllString( + strings.Join(lo.Compact([]string{ref.Owner, ref.Repo, name}), "."), + " ", + ) + return plugin, nil +} From afca7ee5d4401b24a7fe706169aeebf2cc8362b8 Mon Sep 17 00:00:00 2001 From: Mike Landau Date: Thu, 14 Mar 2024 14:43:07 -0700 Subject: [PATCH 5/5] Rename --- internal/plugin/includable.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/plugin/includable.go b/internal/plugin/includable.go index 02e301222d7..77045da139d 100644 --- a/internal/plugin/includable.go +++ b/internal/plugin/includable.go @@ -31,7 +31,7 @@ func parseIncludable(includableRef, workingDir string) (Includable, error) { } } -type fetchable interface { +type fetcher interface { Includable Fetch() ([]byte, error) } @@ -41,7 +41,7 @@ var ( errNameMissing = usererr.New("'name' is missing") ) -func getPluginNameFromContent(plugin fetchable) (string, error) { +func getPluginNameFromContent(plugin fetcher) (string, error) { content, err := plugin.Fetch() if err != nil { return "", err