[plugins] Fix github canonical name (#1897)

## Summary

Fixes #1848

github canonical names were `owner-repo` but this breaks when there are
multiple plugins in a single repo and creates possible conflicts with
other plugins.

This renames them to:

`owner.repo.name` 

where `name` is the name in the plugin.json. For backward compatibility
we don't require the name to be present, if it's missing the name is the
directory (with slashes replaced with hyphens)

## How was it tested?

Installed a few plugins from the same repo and inspected virtenv dir.

Author: mikeland73

internal/plugin/github.go

@@ -5,22 +5,49 @@ import (
 	"io"
 	"net/http"
 	"net/url"
+	"regexp"
+	"strings"
 
+	"github.com/pkg/errors"
+	"github.com/samber/lo"
 	"go.jetpack.io/devbox/internal/boxcli/usererr"
 	"go.jetpack.io/devbox/internal/cachehash"
 	"go.jetpack.io/devbox/nix/flake"
 )
 
 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, 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, err := getPluginNameFromContent(plugin)
+	if err != nil && !errors.Is(err, errNameMissing) {
+		return nil, err
+	}
+	if name == "" {
+		name = strings.ReplaceAll(ref.Dir, "/", "-")
+	}
+	plugin.name = githubNameRegexp.ReplaceAllString(
+		strings.Join(lo.Compact([]string{ref.Owner, ref.Repo, name}), "."),
+		" ",
+	)
+	return plugin, nil
 }
 
 func (p *githubPlugin) Fetch() ([]byte, error) {
 	return p.FileContent(pluginConfigName)
 }
 
 func (p *githubPlugin) CanonicalName() string {
-	return p.ref.Owner + "-" + p.ref.Repo
+	return p.name
 }
 
 func (p *githubPlugin) Hash() string {

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"
 )
@@ -23,6 +25,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 +39,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 +54,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,18 +69,36 @@ 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",
 		},
 	}
 
 	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)
 			assert.Equal(t, testCase.expectedURL, u)
 		})
 	}
 }
+
+// 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
+}

internal/plugin/includable.go

@@ -1,15 +1,18 @@
 package plugin
 
 import (
+	"encoding/json"
 	"fmt"
+	"regexp"
 
+	"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 +25,41 @@ 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)
 	default:
 		return nil, fmt.Errorf("unsupported ref type %q", ref.Type)
 	}
 }
+
+type fetcher interface {
+	Includable
+	Fetch() ([]byte, error)
+}
+
+var (
+	nameRegex      = regexp.MustCompile(`^[a-zA-Z0-9_\- ]+$`)
+	errNameMissing = usererr.New("'name' is missing")
+)
+
+func getPluginNameFromContent(plugin fetcher) (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 "",
+			fmt.Errorf("%w in plugin %s", errNameMissing, 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
+}

internal/plugin/local.go

@@ -1,13 +1,10 @@
 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"
 )
@@ -18,29 +15,12 @@ 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}
-	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
 }