Plugins: Tidy up CLI code (#67813)

* more tidying

* move some things around

* more tidying

* fix linter

pkg/plugins/manager/loader/finder/local.go

@@ -20,7 +20,6 @@ import (
 var walk = util.Walk
 
 var (
-	ErrInvalidPluginJSON         = errors.New("did not find valid type or id properties in plugin.json")
 	ErrInvalidPluginJSONFilePath = errors.New("invalid plugin.json filepath was provided")
 )
 
@@ -149,7 +148,7 @@ func (l *Local) readPluginJSON(pluginJSONPath string) (plugins.JSONData, error)
 		l.log.Warn("Skipping plugin loading as its plugin.json could not be read", "path", pluginJSONPath, "err", err)
 		return plugins.JSONData{}, err
 	}
-	plugin, err := ReadPluginJSON(reader)
+	plugin, err := plugins.ReadPluginJSON(reader)
 	if err != nil {
 		l.log.Warn("Skipping plugin loading as its plugin.json could not be read", "path", pluginJSONPath, "err", err)
 		return plugins.JSONData{}, err

pkg/plugins/manager/loader/finder/local_test.go

@@ -15,7 +15,6 @@ import (
 	"github.com/grafana/grafana/pkg/plugins"
 	"github.com/grafana/grafana/pkg/plugins/manager/fakes"
 	"github.com/grafana/grafana/pkg/services/featuremgmt"
-	"github.com/grafana/grafana/pkg/services/org"
 	"github.com/grafana/grafana/pkg/services/pluginsintegration/config"
 	"github.com/grafana/grafana/pkg/setting"
 	"github.com/grafana/grafana/pkg/util"
@@ -331,127 +330,6 @@ func TestFinder_getAbsPluginJSONPaths(t *testing.T) {
 	})
 }
 
-func TestFinder_validatePluginJSON(t *testing.T) {
-	type args struct {
-		data plugins.JSONData
-	}
-	tests := []struct {
-		name string
-		args args
-		err  error
-	}{
-		{
-			name: "Valid case",
-			args: args{
-				data: plugins.JSONData{
-					ID:   "grafana-plugin-id",
-					Type: plugins.DataSource,
-				},
-			},
-		},
-		{
-			name: "Invalid plugin ID",
-			args: args{
-				data: plugins.JSONData{
-					Type: plugins.Panel,
-				},
-			},
-			err: ErrInvalidPluginJSON,
-		},
-		{
-			name: "Invalid plugin type",
-			args: args{
-				data: plugins.JSONData{
-					ID:   "grafana-plugin-id",
-					Type: "test",
-				},
-			},
-			err: ErrInvalidPluginJSON,
-		},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			if err := validatePluginJSON(tt.args.data); !errors.Is(err, tt.err) {
-				t.Errorf("validatePluginJSON() = %v, want %v", err, tt.err)
-			}
-		})
-	}
-}
-
-func TestFinder_readPluginJSON(t *testing.T) {
-	tests := []struct {
-		name       string
-		pluginPath string
-		expected   plugins.JSONData
-		err        error
-	}{
-		{
-			name:       "Valid plugin",
-			pluginPath: "../../testdata/test-app/plugin.json",
-			expected: plugins.JSONData{
-				ID:   "test-app",
-				Type: "app",
-				Name: "Test App",
-				Info: plugins.Info{
-					Author: plugins.InfoLink{
-						Name: "Test Inc.",
-						URL:  "http://test.com",
-					},
-					Description: "Official Grafana Test App & Dashboard bundle",
-					Version:     "1.0.0",
-					Links: []plugins.InfoLink{
-						{Name: "Project site", URL: "http://project.com"},
-						{Name: "License & Terms", URL: "http://license.com"},
-					},
-					Logos: plugins.Logos{
-						Small: "img/logo_small.png",
-						Large: "img/logo_large.png",
-					},
-					Screenshots: []plugins.Screenshots{
-						{Path: "img/screenshot1.png", Name: "img1"},
-						{Path: "img/screenshot2.png", Name: "img2"},
-					},
-					Updated: "2015-02-10",
-				},
-				Dependencies: plugins.Dependencies{
-					GrafanaVersion: "3.x.x",
-					Plugins: []plugins.Dependency{
-						{Type: "datasource", ID: "graphite", Name: "Graphite", Version: "1.0.0"},
-						{Type: "panel", ID: "graph", Name: "Graph", Version: "1.0.0"},
-					},
-				},
-				Includes: []*plugins.Includes{
-					{Name: "Nginx Connections", Path: "dashboards/connections.json", Type: "dashboard", Role: org.RoleViewer},
-					{Name: "Nginx Memory", Path: "dashboards/memory.json", Type: "dashboard", Role: org.RoleViewer},
-					{Name: "Nginx Panel", Type: "panel", Role: org.RoleViewer},
-					{Name: "Nginx Datasource", Type: "datasource", Role: org.RoleViewer},
-				},
-				Backend: false,
-			},
-		},
-		{
-			name:       "Invalid plugin JSON",
-			pluginPath: "../../testdata/invalid-plugin-json/plugin.json",
-			err:        ErrInvalidPluginJSON,
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			reader, err := os.Open(tt.pluginPath)
-			require.NoError(t, err)
-			got, err := ReadPluginJSON(reader)
-			if tt.err != nil {
-				require.ErrorIs(t, err, tt.err)
-			}
-			if !cmp.Equal(got, tt.expected) {
-				t.Errorf("Unexpected pluginJSONData: %v", cmp.Diff(got, tt.expected))
-			}
-			require.NoError(t, reader.Close())
-		})
-	}
-}
-
 var fsComparer = cmp.Comparer(func(fs1 plugins.FS, fs2 plugins.FS) bool {
 	fs1Files, err := fs1.Files()
 	if err != nil {

pkg/plugins/manager/loader/finder/util.go

@@ -1,47 +0,0 @@
-package finder
-
-import (
-	"encoding/json"
-	"io"
-
-	"github.com/grafana/grafana/pkg/plugins"
-	"github.com/grafana/grafana/pkg/services/org"
-)
-
-func ReadPluginJSON(reader io.Reader) (plugins.JSONData, error) {
-	plugin := plugins.JSONData{}
-	if err := json.NewDecoder(reader).Decode(&plugin); err != nil {
-		return plugins.JSONData{}, err
-	}
-
-	if err := validatePluginJSON(plugin); err != nil {
-		return plugins.JSONData{}, err
-	}
-
-	if plugin.ID == "grafana-piechart-panel" {
-		plugin.Name = "Pie Chart (old)"
-	}
-
-	if len(plugin.Dependencies.Plugins) == 0 {
-		plugin.Dependencies.Plugins = []plugins.Dependency{}
-	}
-
-	if plugin.Dependencies.GrafanaVersion == "" {
-		plugin.Dependencies.GrafanaVersion = "*"
-	}
-
-	for _, include := range plugin.Includes {
-		if include.Role == "" {
-			include.Role = org.RoleViewer
-		}
-	}
-
-	return plugin, nil
-}
-
-func validatePluginJSON(data plugins.JSONData) error {
-	if data.ID == "" || !data.Type.IsValid() {
-		return ErrInvalidPluginJSON
-	}
-	return nil
-}

pkg/plugins/plugins.go

@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"io"
 	"io/fs"
 	"path"
 	"runtime"
@@ -24,6 +25,7 @@ var (
 	ErrFileNotExist              = errors.New("file does not exist")
 	ErrPluginFileRead            = errors.New("file could not be read")
 	ErrUninstallInvalidPluginDir = errors.New("cannot recognize as plugin folder")
+	ErrInvalidPluginJSON         = errors.New("did not find valid type or id properties in plugin.json")
 )
 
 type Plugin struct {
@@ -139,6 +141,44 @@ type JSONData struct {
 	Executable string `json:"executable,omitempty"`
 }
 
+func ReadPluginJSON(reader io.Reader) (JSONData, error) {
+	plugin := JSONData{}
+	if err := json.NewDecoder(reader).Decode(&plugin); err != nil {
+		return JSONData{}, err
+	}
+
+	if err := validatePluginJSON(plugin); err != nil {
+		return JSONData{}, err
+	}
+
+	if plugin.ID == "grafana-piechart-panel" {
+		plugin.Name = "Pie Chart (old)"
+	}
+
+	if len(plugin.Dependencies.Plugins) == 0 {
+		plugin.Dependencies.Plugins = []Dependency{}
+	}
+
+	if plugin.Dependencies.GrafanaVersion == "" {
+		plugin.Dependencies.GrafanaVersion = "*"
+	}
+
+	for _, include := range plugin.Includes {
+		if include.Role == "" {
+			include.Role = org.RoleViewer
+		}
+	}
+
+	return plugin, nil
+}
+
+func validatePluginJSON(data JSONData) error {
+	if data.ID == "" || !data.Type.IsValid() {
+		return ErrInvalidPluginJSON
+	}
+	return nil
+}
+
 func (d JSONData) DashboardIncludes() []*Includes {
 	result := []*Includes{}
 	for _, include := range d.Includes {

pkg/plugins/plugins_test.go

@@ -0,0 +1,167 @@
+package plugins
+
+import (
+	"errors"
+	"io"
+	"os"
+	"strings"
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	"github.com/grafana/grafana/pkg/services/org"
+	"github.com/stretchr/testify/require"
+)
+
+func Test_ReadPluginJSON(t *testing.T) {
+	tests := []struct {
+		name       string
+		pluginJSON func(t *testing.T) io.ReadCloser
+		expected   JSONData
+		err        error
+	}{
+		{
+			name: "Valid plugin",
+			pluginJSON: func(t *testing.T) io.ReadCloser {
+				reader, err := os.Open("manager/testdata/test-app/plugin.json")
+				require.NoError(t, err)
+				return reader
+			},
+			expected: JSONData{
+				ID:   "test-app",
+				Type: "app",
+				Name: "Test App",
+				Info: Info{
+					Author: InfoLink{
+						Name: "Test Inc.",
+						URL:  "http://test.com",
+					},
+					Description: "Official Grafana Test App & Dashboard bundle",
+					Version:     "1.0.0",
+					Links: []InfoLink{
+						{Name: "Project site", URL: "http://project.com"},
+						{Name: "License & Terms", URL: "http://license.com"},
+					},
+					Logos: Logos{
+						Small: "img/logo_small.png",
+						Large: "img/logo_large.png",
+					},
+					Screenshots: []Screenshots{
+						{Path: "img/screenshot1.png", Name: "img1"},
+						{Path: "img/screenshot2.png", Name: "img2"},
+					},
+					Updated: "2015-02-10",
+				},
+				Dependencies: Dependencies{
+					GrafanaVersion: "3.x.x",
+					Plugins: []Dependency{
+						{Type: "datasource", ID: "graphite", Name: "Graphite", Version: "1.0.0"},
+						{Type: "panel", ID: "graph", Name: "Graph", Version: "1.0.0"},
+					},
+				},
+				Includes: []*Includes{
+					{Name: "Nginx Connections", Path: "dashboards/connections.json", Type: "dashboard", Role: org.RoleViewer},
+					{Name: "Nginx Memory", Path: "dashboards/memory.json", Type: "dashboard", Role: org.RoleViewer},
+					{Name: "Nginx Panel", Type: "panel", Role: org.RoleViewer},
+					{Name: "Nginx Datasource", Type: "datasource", Role: org.RoleViewer},
+				},
+				Backend: false,
+			},
+		},
+		{
+			name: "Invalid plugin JSON",
+			pluginJSON: func(t *testing.T) io.ReadCloser {
+				reader, err := os.Open("manager/testdata/invalid-plugin-json/plugin.json")
+				require.NoError(t, err)
+				return reader
+			},
+			err: ErrInvalidPluginJSON,
+		},
+		{
+			name: "Default value overrides",
+			pluginJSON: func(t *testing.T) io.ReadCloser {
+				pJSON := `{
+					"id": "grafana-piechart-panel",
+					"name": "This will be overwritten",
+					"type": "panel",
+					"includes": [
+						{"type": "dashboard", "name": "Pie Charts", "path": "dashboards/demo.json"}
+					]
+				}`
+				return io.NopCloser(strings.NewReader(pJSON))
+			},
+			expected: JSONData{
+				ID:   "grafana-piechart-panel",
+				Type: "panel",
+				Name: "Pie Chart (old)",
+				Dependencies: Dependencies{
+					GrafanaVersion: "*",
+					Plugins:        []Dependency{},
+				},
+				Includes: []*Includes{
+					{Name: "Pie Charts", Path: "dashboards/demo.json", Type: "dashboard", Role: org.RoleViewer},
+				},
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			p := tt.pluginJSON(t)
+			got, err := ReadPluginJSON(p)
+			if tt.err != nil {
+				require.ErrorIs(t, err, tt.err)
+			}
+			if !cmp.Equal(got, tt.expected) {
+				t.Errorf("Unexpected pluginJSONData: %v", cmp.Diff(got, tt.expected))
+			}
+			require.NoError(t, p.Close())
+		})
+	}
+}
+
+func Test_validatePluginJSON(t *testing.T) {
+	type args struct {
+		data JSONData
+	}
+	tests := []struct {
+		name string
+		args args
+		err  error
+	}{
+		{
+			name: "Valid case",
+			args: args{
+				data: JSONData{
+					ID:   "grafana-plugin-id",
+					Type: DataSource,
+				},
+			},
+		},
+		{
+			name: "Invalid plugin ID",
+			args: args{
+				data: JSONData{
+					Type: Panel,
+				},
+			},
+			err: ErrInvalidPluginJSON,
+		},
+		{
+			name: "Invalid plugin type",
+			args: args{
+				data: JSONData{
+					ID:   "grafana-plugin-id",
+					Type: "test",
+				},
+			},
+			err: ErrInvalidPluginJSON,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if err := validatePluginJSON(tt.args.data); !errors.Is(err, tt.err) {
+				t.Errorf("validatePluginJSON() = %v, want %v", err, tt.err)
+			}
+		})
+	}
+}

pkg/plugins/storage/fs.go

@@ -4,7 +4,6 @@ import (
 	"archive/zip"
 	"bytes"
 	"context"
-	"encoding/json"
 	"errors"
 	"fmt"
 	"io"
@@ -13,6 +12,7 @@ import (
 	"regexp"
 	"strings"
 
+	"github.com/grafana/grafana/pkg/plugins"
 	"github.com/grafana/grafana/pkg/plugins/log"
 )
 
@@ -39,15 +39,15 @@ func (fs *FS) Extract(ctx context.Context, pluginID string, pluginArchive *zip.R
 		return nil, fmt.Errorf("%v: %w", "failed to extract plugin archive", err)
 	}
 
-	res, err := toPluginDTO(pluginID, pluginDir)
+	pluginJSON, err := readPluginJSON(pluginID, pluginDir)
 	if err != nil {
 		return nil, fmt.Errorf("%v: %w", "failed to convert to plugin DTO", err)
 	}
 
-	fs.log.Successf("Downloaded and extracted %s v%s zip successfully to %s", res.ID, res.Info.Version, pluginDir)
+	fs.log.Successf("Downloaded and extracted %s v%s zip successfully to %s", pluginJSON.ID, pluginJSON.Info.Version, pluginDir)
 
-	deps := make([]*Dependency, 0, len(res.Dependencies.Plugins))
-	for _, plugin := range res.Dependencies.Plugins {
+	deps := make([]*Dependency, 0, len(pluginJSON.Dependencies.Plugins))
+	for _, plugin := range pluginJSON.Dependencies.Plugins {
 		deps = append(deps, &Dependency{
 			ID:      plugin.ID,
 			Version: plugin.Version,
@@ -55,8 +55,8 @@ func (fs *FS) Extract(ctx context.Context, pluginID string, pluginArchive *zip.R
 	}
 
 	return &ExtractedPluginArchive{
-		ID:           res.ID,
-		Version:      res.Info.Version,
+		ID:           pluginJSON.ID,
+		Version:      pluginJSON.Info.Version,
 		Dependencies: deps,
 		Path:         pluginDir,
 	}, nil
@@ -220,34 +220,26 @@ func removeGitBuildFromName(filename, pluginID string) string {
 	return reGitBuild.ReplaceAllString(filename, pluginID+"/")
 }
 
-func toPluginDTO(pluginID, pluginDir string) (installedPlugin, error) {
-	distPluginDataPath := filepath.Join(pluginDir, "dist", "plugin.json")
+func readPluginJSON(pluginID, pluginDir string) (plugins.JSONData, error) {
+	pluginPath := filepath.Join(pluginDir, "plugin.json")
 
 	// It's safe to ignore gosec warning G304 since the file path suffix is hardcoded
 	// nolint:gosec
-	data, err := os.ReadFile(distPluginDataPath)
+	data, err := os.ReadFile(pluginPath)
 	if err != nil {
-		pluginDataPath := filepath.Join(pluginDir, "plugin.json")
+		pluginPath = filepath.Join(pluginDir, "dist", "plugin.json")
 		// It's safe to ignore gosec warning G304 since the file path suffix is hardcoded
 		// nolint:gosec
-		data, err = os.ReadFile(pluginDataPath)
+		data, err = os.ReadFile(pluginPath)
 		if err != nil {
-			return installedPlugin{}, fmt.Errorf("could not find dist/plugin.json or plugin.json for %s in %s", pluginID, pluginDir)
+			return plugins.JSONData{}, fmt.Errorf("could not find plugin.json or dist/plugin.json for %s in %s", pluginID, pluginDir)
 		}
 	}
 
-	res := installedPlugin{}
-	if err = json.Unmarshal(data, &res); err != nil {
-		return res, err
+	pJSON, err := plugins.ReadPluginJSON(bytes.NewReader(data))
+	if err != nil {
+		return plugins.JSONData{}, err
 	}
 
-	if res.ID == "" {
-		return installedPlugin{}, fmt.Errorf("could not find valid plugin %s in %s", pluginID, pluginDir)
-	}
-
-	if res.Info.Version == "" {
-		res.Info.Version = "0.0.0"
-	}
-
-	return res, nil
+	return pJSON, nil
 }

pkg/plugins/storage/models.go

@@ -21,28 +21,3 @@ type Dependency struct {
 	ID      string
 	Version string
 }
-
-type installedPlugin struct {
-	ID           string       `json:"id"`
-	Name         string       `json:"name"`
-	Type         string       `json:"type"`
-	Info         pluginInfo   `json:"info"`
-	Dependencies dependencies `json:"dependencies"`
-}
-
-type dependencies struct {
-	GrafanaVersion string             `json:"grafanaVersion"`
-	Plugins        []pluginDependency `json:"plugins"`
-}
-
-type pluginDependency struct {
-	ID      string `json:"id"`
-	Type    string `json:"type"`
-	Name    string `json:"name"`
-	Version string `json:"version"`
-}
-
-type pluginInfo struct {
-	Version string `json:"version"`
-	Updated string `json:"updated"`
-}