From babbc19485b55baa0135a595a9c64a7f666947b4 Mon Sep 17 00:00:00 2001 From: Yuriy Hrytsyuk Date: Tue, 10 Feb 2026 14:58:31 +0200 Subject: [PATCH 1/3] [CFX-4730] Add CLI version constraints to plugin manifest --- cmd/root.go | 2 +- docs/development/plugins.md | 2 +- docs/user-guide/configuration.md | 2 +- internal/plugin/discover.go | 39 +++++++++++++++++++++++++++-- internal/plugin/integration_test.go | 6 ++--- internal/plugin/remote.go | 1 - internal/plugin/remote_test.go | 4 +-- internal/plugin/types.go | 4 +-- internal/plugin/validation.go | 6 ++--- internal/plugin/validation_test.go | 22 ++++++++-------- 10 files changed, 61 insertions(+), 27 deletions(-) diff --git a/cmd/root.go b/cmd/root.go index d3ece66e..76e0fdbf 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -104,7 +104,7 @@ func init() { RootCmd.PersistentFlags().Bool("all-commands", false, "display all available commands and their flags in tree format") RootCmd.PersistentFlags().Bool("skip-auth", false, "skip authentication checks (for advanced users)") RootCmd.PersistentFlags().Bool("force-interactive", false, "force setup wizards to run even if already completed") - RootCmd.PersistentFlags().Duration("plugin-discovery-timeout", 2*time.Second, "timeout for plugin discovery (0s disables)") + RootCmd.PersistentFlags().Duration("plugin-discovery-timeout", 3*time.Second, "timeout for plugin discovery (0s disables)") // Make some of these flags available via Viper _ = viper.BindPFlag("config", RootCmd.PersistentFlags().Lookup("config")) diff --git a/docs/development/plugins.md b/docs/development/plugins.md index a20fb31a..05ed6a3e 100644 --- a/docs/development/plugins.md +++ b/docs/development/plugins.md @@ -30,7 +30,7 @@ Plugins are deduplicated by `manifest.name` (not by filename). If multiple binar ### Timeouts -- Overall discovery is bounded by the global flag `--plugin-discovery-timeout` (default `2s`). +- Overall discovery is bounded by the global flag `--plugin-discovery-timeout` (default `3s`). - Set to `0s` to disable plugin discovery entirely. - Manifest retrieval is bounded by `plugin.manifest_timeout_ms` (default `500ms`). diff --git a/docs/user-guide/configuration.md b/docs/user-guide/configuration.md index 67df955f..fd77c208 100644 --- a/docs/user-guide/configuration.md +++ b/docs/user-guide/configuration.md @@ -115,7 +115,7 @@ dr templates list --verbose dr templates list --debug # Timeout for plugin discovery (0s disables discovery) -dr --plugin-discovery-timeout 2s --help +dr --plugin-discovery-timeout 3s --help ``` > [!WARNING] diff --git a/internal/plugin/discover.go b/internal/plugin/discover.go index 3368142c..aeac2d7d 100644 --- a/internal/plugin/discover.go +++ b/internal/plugin/discover.go @@ -25,8 +25,10 @@ import ( "strings" "time" + "github.com/Masterminds/semver/v3" "github.com/datarobot/cli/internal/log" "github.com/datarobot/cli/internal/repo" + "github.com/datarobot/cli/internal/version" "github.com/spf13/viper" ) @@ -205,7 +207,7 @@ func errMissingManifestField(field string) error { return errors.New("plugin manifest missing required field: " + field) } -func discoverInDir(dir string, seen map[string]bool) ([]DiscoveredPlugin, []error) { +func discoverInDir(dir string, seen map[string]bool) ([]DiscoveredPlugin, []error) { //nolint: cyclop plugins := make([]DiscoveredPlugin, 0) var errors []error @@ -242,7 +244,6 @@ func discoverInDir(dir string, seen map[string]bool) ([]DiscoveredPlugin, []erro manifest, err := getManifest(fullPath) if err != nil { errors = append(errors, err) - continue } @@ -255,6 +256,21 @@ func discoverInDir(dir string, seen map[string]bool) ([]DiscoveredPlugin, []erro continue } + compatible, err := compatibleCLIVersion(version.Version, manifest.CLIVersion) + if err != nil { + log.Warn("Cannot parse cli version constraint for plugin", + "name", manifest.Name, + "installed", version.Version, + "constraint", manifest.CLIVersion) + } else if !compatible { + log.Warn("Plugin is incompatible with DataRobot CLI version", + "name", manifest.Name, + "installed", version.Version, + "constraint", manifest.CLIVersion) + + continue + } + seen[manifest.Name] = true plugins = append(plugins, DiscoveredPlugin{ @@ -299,3 +315,22 @@ func getManifest(executable string) (*PluginManifest, error) { return &manifest, nil } + +func compatibleCLIVersion(cliVersion, constraint string) (bool, error) { + if cliVersion == "dev" || constraint == "" { + return true, nil + } + + c, err := semver.NewConstraint(constraint) + if err != nil { + return false, err + } + + v, err := semver.NewVersion(cliVersion) + if err != nil { + return false, err + } + + // Check if the version meets the constraints. + return c.Check(v), nil +} diff --git a/internal/plugin/integration_test.go b/internal/plugin/integration_test.go index 37fdd6ca..738aa6d4 100644 --- a/internal/plugin/integration_test.go +++ b/internal/plugin/integration_test.go @@ -88,9 +88,9 @@ func TestLivePluginManifests(t *testing.T) { // Validate version format assert.Regexp(t, `^v?\d+\.\d+\.\d+`, manifest.Version, "Manifest %s version must be semver", manifestPath) - // Validate minCLIVersion format if present - if manifest.MinCLIVersion != "" { - assert.Regexp(t, `^v?\d+\.\d+\.\d+`, manifest.MinCLIVersion, "Manifest %s minCLIVersion must be semver", manifestPath) + // Validate CLIVersion format if present + if manifest.CLIVersion != "" { + assert.Regexp(t, `^v?\d+\.\d+\.\d+`, manifest.CLIVersion, "Manifest %s CLIVersion must be semver", manifestPath) } }) } diff --git a/internal/plugin/remote.go b/internal/plugin/remote.go index fae0221b..313109bd 100644 --- a/internal/plugin/remote.go +++ b/internal/plugin/remote.go @@ -30,7 +30,6 @@ import ( "time" "github.com/Masterminds/semver/v3" - "github.com/codeclysm/extract/v4" "github.com/datarobot/cli/internal/config" "github.com/datarobot/cli/internal/log" diff --git a/internal/plugin/remote_test.go b/internal/plugin/remote_test.go index 381a628d..bce763e2 100644 --- a/internal/plugin/remote_test.go +++ b/internal/plugin/remote_test.go @@ -109,7 +109,7 @@ func TestPluginManifestSchema(t *testing.T) { "name":"test", "version":"1.0.0", "description":"Test plugin", - "minCLIVersion":"0.2.0", + "cliVersion":"0.2.0", "authentication":true, "scripts":{ "posix":"scripts/test.sh", @@ -120,7 +120,7 @@ func TestPluginManifestSchema(t *testing.T) { assert.Equal(t, "test", m.Name) assert.Equal(t, "1.0.0", m.Version) assert.Equal(t, "Test plugin", m.Description) - assert.Equal(t, "0.2.0", m.MinCLIVersion) + assert.Equal(t, "0.2.0", m.CLIVersion) assert.True(t, m.Authentication) require.NotNil(t, m.Scripts) assert.Equal(t, "scripts/test.sh", m.Scripts.Posix) diff --git a/internal/plugin/types.go b/internal/plugin/types.go index b632d282..a05ea23a 100644 --- a/internal/plugin/types.go +++ b/internal/plugin/types.go @@ -40,8 +40,8 @@ type BasicPluginManifest struct { // Embeds BasicPluginManifest and adds additional fields for managed plugins. type PluginManifest struct { BasicPluginManifest - Scripts *PluginScripts `json:"scripts,omitempty"` // Platform-specific script paths - MinCLIVersion string `json:"minCLIVersion,omitempty"` // Minimum CLI version required + Scripts *PluginScripts `json:"scripts,omitempty"` // Platform-specific script paths + CLIVersion string `json:"cliVersion,omitempty"` // Minimum CLI version required } // RegistryVersion represents a specific version in the plugin registry diff --git a/internal/plugin/validation.go b/internal/plugin/validation.go index 7265c737..89df79d1 100644 --- a/internal/plugin/validation.go +++ b/internal/plugin/validation.go @@ -29,7 +29,7 @@ import ( ) // ValidatePluginScript validates that a plugin script outputs a manifest matching the expected manifest. -// All fields must match exactly, including Scripts and MinCLIVersion for managed plugins. +// All fields must match exactly, including Scripts and CLIVersion for managed plugins. func ValidatePluginScript(pluginDir string, expectedManifest PluginManifest) error { if err := ValidateLicense(pluginDir); err != nil { return err @@ -70,9 +70,9 @@ func ValidateLicense(pluginDir string) error { // validateManifests compares two manifests and returns an error if they differ. func validateManifests(expected, actual PluginManifest) error { opts := cmp.Options{ - // Ignore Scripts and MinCLIVersion - they're optional managed plugin fields + // Ignore Scripts and CLIVersion - they're optional managed plugin fields cmp.FilterPath(func(p cmp.Path) bool { - return p.String() == "Scripts" || p.String() == "MinCLIVersion" + return p.String() == "Scripts" || p.String() == "CLIVersion" }, cmp.Ignore()), } diff --git a/internal/plugin/validation_test.go b/internal/plugin/validation_test.go index a8f621d8..fd21e12c 100644 --- a/internal/plugin/validation_test.go +++ b/internal/plugin/validation_test.go @@ -33,7 +33,7 @@ func TestValidateManifests_Matching(t *testing.T) { Description: "Test plugin", Authentication: true, }, - MinCLIVersion: "1.0.0", + CLIVersion: "1.0.0", } err := validateManifests(manifest, manifest) @@ -49,7 +49,7 @@ func TestValidateManifests_Mismatch(t *testing.T) { Description: "Test plugin", Authentication: true, }, - MinCLIVersion: "1.0.0", + CLIVersion: "1.0.0", } actual := PluginManifest{ @@ -59,7 +59,7 @@ func TestValidateManifests_Mismatch(t *testing.T) { Description: "Different description", Authentication: false, }, - MinCLIVersion: "1.0.0", + CLIVersion: "1.0.0", } err := validateManifests(expected, actual) @@ -83,7 +83,7 @@ func TestValidateManifests_ManagedPlugin(t *testing.T) { Posix: "dr-apps.sh", Windows: "dr-apps.ps1", }, - MinCLIVersion: "0.2.0", + CLIVersion: "0.2.0", } err := validateManifests(manifest, manifest) @@ -92,7 +92,7 @@ func TestValidateManifests_ManagedPlugin(t *testing.T) { } func TestValidateManifests_MissingScripts(t *testing.T) { - // Scripts and MinCLIVersion are now ignored in validation + // Scripts and CLIVersion are now ignored in validation // This allows managed plugins to work as PATH plugins expected := PluginManifest{ BasicPluginManifest: BasicPluginManifest{ @@ -105,7 +105,7 @@ func TestValidateManifests_MissingScripts(t *testing.T) { Posix: "dr-apps.sh", Windows: "dr-apps.ps1", }, - MinCLIVersion: "0.2.0", + CLIVersion: "0.2.0", } actual := PluginManifest{ @@ -115,13 +115,13 @@ func TestValidateManifests_MissingScripts(t *testing.T) { Description: "Host custom applications in DataRobot", Authentication: true, }, - Scripts: nil, // Different but ignored - MinCLIVersion: "", // Different but ignored + Scripts: nil, // Different but ignored + CLIVersion: "", // Different but ignored } err := validateManifests(expected, actual) - assert.NoError(t, err, "Scripts and MinCLIVersion differences should be ignored") + assert.NoError(t, err, "Scripts and CLIVersion differences should be ignored") } func TestValidateManifests_AllowExtraFields(t *testing.T) { @@ -147,7 +147,7 @@ func TestValidateManifests_AllowExtraFields(t *testing.T) { Posix: "dr-apps.sh", Windows: "dr-apps.ps1", }, - MinCLIVersion: "0.2.0", + CLIVersion: "0.2.0", } // Should pass - actual can have more fields than expected @@ -246,7 +246,7 @@ func TestExecPluginManifest(t *testing.T) { Description: "Test plugin", Authentication: true, }, - MinCLIVersion: "1.0.0", + CLIVersion: "1.0.0", } scriptPath := createTestScript(t, tempDir, expected) From 1c7dcc63146d5a5cf8d870bb0a791a7a661742aa Mon Sep 17 00:00:00 2001 From: AJ Alon Date: Fri, 20 Feb 2026 17:36:34 -0800 Subject: [PATCH 2/3] feat(discover): check CLI version constraint for managed plugins feat(discover): with tests --- internal/plugin/discover.go | 15 ++ internal/plugin/discover_test.go | 387 ++++++++++++------------------- 2 files changed, 160 insertions(+), 242 deletions(-) diff --git a/internal/plugin/discover.go b/internal/plugin/discover.go index aeac2d7d..9845c2ec 100644 --- a/internal/plugin/discover.go +++ b/internal/plugin/discover.go @@ -162,6 +162,21 @@ func loadManagedPlugin(dir, name string, seen map[string]bool) (*DiscoveredPlugi return nil, nil } + compatible, err := compatibleCLIVersion(version.Version, manifest.CLIVersion) + if err != nil { + log.Warn("Cannot parse cli version constraint for plugin", + "name", manifest.Name, + "installed", version.Version, + "constraint", manifest.CLIVersion) + } else if !compatible { + log.Warn("Plugin is incompatible with DataRobot CLI version", + "name", manifest.Name, + "installed", version.Version, + "constraint", manifest.CLIVersion) + + return nil, nil + } + executable, err := resolvePlatformExecutable(pluginDir, &manifest) if err != nil { return nil, err diff --git a/internal/plugin/discover_test.go b/internal/plugin/discover_test.go index 7da4bb98..17a3a693 100644 --- a/internal/plugin/discover_test.go +++ b/internal/plugin/discover_test.go @@ -15,256 +15,159 @@ package plugin import ( - "fmt" - "os" - "path/filepath" "testing" - "github.com/spf13/viper" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "github.com/stretchr/testify/suite" ) -// Test manifests -var ( - validManifest = `{"name":"test-plugin","version":"1.0.0","description":"Test plugin"}` - invalidManifest = `{invalid json` -) - -// createMockPlugin creates a shell script that responds to --dr-plugin-manifest -func createMockPlugin(t *testing.T, dir, name, manifestJSON string) string { - t.Helper() - - script := fmt.Sprintf(`#!/bin/sh -if [ "$1" = "--dr-plugin-manifest" ]; then - echo '%s' -else - exit 0 -fi -`, manifestJSON) - - path := filepath.Join(dir, name) - err := os.WriteFile(path, []byte(script), 0o755) - require.NoError(t, err) - - return path -} - -// DiscoverTestSuite tests discovery functions with filesystem fixtures -type DiscoverTestSuite struct { - suite.Suite - tempDir string -} - -func TestDiscoverTestSuite(t *testing.T) { - suite.Run(t, new(DiscoverTestSuite)) -} - -func (s *DiscoverTestSuite) SetupTest() { - var err error - - s.tempDir, err = os.MkdirTemp("", "plugin-discover-test") - s.Require().NoError(err) -} - -func (s *DiscoverTestSuite) TearDownTest() { - if s.tempDir != "" { - _ = os.RemoveAll(s.tempDir) - } -} - -func (s *DiscoverTestSuite) TestDiscoverInDirEmptyDirectory() { - seen := make(map[string]bool) - plugins, errs := discoverInDir(s.tempDir, seen) - - s.Empty(plugins) - s.Empty(errs) -} - -func (s *DiscoverTestSuite) TestDiscoverInDirNonExistent() { - seen := make(map[string]bool) - plugins, errs := discoverInDir(filepath.Join(s.tempDir, "nonexistent"), seen) - - s.Nil(plugins) - s.Nil(errs) -} - -func (s *DiscoverTestSuite) TestDiscoverInDirValidPlugin() { - createMockPlugin(s.T(), s.tempDir, "dr-testplugin", validManifest) - - seen := make(map[string]bool) - plugins, errs := discoverInDir(s.tempDir, seen) - - s.Require().Len(plugins, 1, "Expected exactly one plugin") - s.Empty(errs) - s.Equal("test-plugin", plugins[0].Manifest.Name) - s.Equal("1.0.0", plugins[0].Manifest.Version) - s.Equal("Test plugin", plugins[0].Manifest.Description) - s.True(seen["test-plugin"]) // seen map now uses manifest.Name -} - -func (s *DiscoverTestSuite) TestDiscoverInDirSkipsNonDrFiles() { - // Create a valid plugin - createMockPlugin(s.T(), s.tempDir, "dr-valid", validManifest) - - // Create non-dr files - s.Require().NoError(os.WriteFile(filepath.Join(s.tempDir, "other-binary"), []byte("#!/bin/sh\nexit 0"), 0o755)) - s.Require().NoError(os.WriteFile(filepath.Join(s.tempDir, "random.txt"), []byte("text"), 0o644)) - - seen := make(map[string]bool) - plugins, _ := discoverInDir(s.tempDir, seen) - - s.Require().Len(plugins, 1, "Expected exactly one plugin (non-dr files should be skipped)") - s.Equal("test-plugin", plugins[0].Manifest.Name) -} - -func (s *DiscoverTestSuite) TestDiscoverInDirSkipsNonExecutable() { - // Create a dr-prefixed file that is not executable - path := filepath.Join(s.tempDir, "dr-notexec") - s.Require().NoError(os.WriteFile(path, []byte("not executable"), 0o644)) - - seen := make(map[string]bool) - plugins, errs := discoverInDir(s.tempDir, seen) - - s.Empty(plugins) - s.Empty(errs) -} - -func (s *DiscoverTestSuite) TestDiscoverInDirHandlesDuplicates() { - createMockPlugin(s.T(), s.tempDir, "dr-duplicate", validManifest) - - // Pre-populate seen map with manifest name (not filename) - seen := map[string]bool{ - "test-plugin": true, // validManifest has name: "test-plugin" - } - - plugins, _ := discoverInDir(s.tempDir, seen) - - // Should be skipped due to duplicate manifest name - s.Empty(plugins) -} - -func (s *DiscoverTestSuite) TestDiscoverInDirDeduplicatesByManifestName() { - // Two different binary names, same manifest name - manifest := `{"name":"shared-name","version":"1.0.0","description":"Test"}` - createMockPlugin(s.T(), s.tempDir, "dr-first", manifest) - createMockPlugin(s.T(), s.tempDir, "dr-second", manifest) - - seen := make(map[string]bool) - plugins, errs := discoverInDir(s.tempDir, seen) - - // Only one should be registered (first one wins based on directory order) - s.Require().Len(plugins, 1, "Expected exactly one plugin when two share the same manifest name") - s.Empty(errs) - s.Equal("shared-name", plugins[0].Manifest.Name) -} - -func (s *DiscoverTestSuite) TestDiscoverInDirInvalidManifest() { - createMockPlugin(s.T(), s.tempDir, "dr-invalid", invalidManifest) - - seen := make(map[string]bool) - plugins, errs := discoverInDir(s.tempDir, seen) - - s.Empty(plugins) - s.Len(errs, 1) // JSON parse error logged -} - -func (s *DiscoverTestSuite) TestDiscoverInDirMultipleValidPlugins() { - manifest1 := `{"name":"plugin-one","version":"1.0.0","description":"First plugin"}` - manifest2 := `{"name":"plugin-two","version":"2.0.0","description":"Second plugin"}` - - createMockPlugin(s.T(), s.tempDir, "dr-one", manifest1) - createMockPlugin(s.T(), s.tempDir, "dr-two", manifest2) - - seen := make(map[string]bool) - plugins, errs := discoverInDir(s.tempDir, seen) - - s.Len(plugins, 2) - s.Empty(errs) - - // Verify both plugins discovered - names := make(map[string]bool) - for _, p := range plugins { - names[p.Manifest.Name] = true +func TestCompatibleCLIVersion(t *testing.T) { + tests := []struct { + name string + cliVersion string + constraint string + compatible bool + expectErr bool + }{ + // No constraint - always compatible + { + name: "empty constraint is always compatible", + cliVersion: "1.0.0", + constraint: "", + compatible: true, + }, + // Dev version - always compatible + { + name: "dev CLI version is always compatible", + cliVersion: "dev", + constraint: ">=1.0.0", + compatible: true, + }, + // Latest CLI + plugin with no CLI constraints + { + name: "latest CLI with no constraints", + cliVersion: "2.0.0", + constraint: "", + compatible: true, + }, + // Latest CLI + plugin with CLI min version (using >= constraint) + { + name: "latest CLI satisfies min version constraint", + cliVersion: "2.0.0", + constraint: ">=1.0.0", + compatible: true, + }, + // Latest CLI + plugin with CLI max version (using < constraint) + { + name: "latest CLI within max version constraint", + cliVersion: "1.5.0", + constraint: "< 2.0.0", + compatible: true, + }, + { + name: "latest CLI exceeds max version constraint", + cliVersion: "2.0.0", + constraint: "< 2.0.0", + compatible: false, + }, + // Old CLI + plugin with no CLI constraints + { + name: "old CLI with no constraints", + cliVersion: "0.1.0", + constraint: "", + compatible: true, + }, + // Old CLI + plugin with CLI min version → incompatible + { + name: "old CLI below min version constraint", + cliVersion: "0.1.0", + constraint: ">=1.0.0", + compatible: false, + }, + // Old CLI + plugin with CLI max version + { + name: "old CLI within max version constraint", + cliVersion: "0.1.0", + constraint: "< 2.0.0", + compatible: true, + }, + // Semver constraint patterns + { + name: "caret constraint compatible", + cliVersion: "1.5.0", + constraint: "^1.0.0", + compatible: true, + }, + { + name: "caret constraint incompatible major bump", + cliVersion: "2.0.0", + constraint: "^1.0.0", + compatible: false, + }, + { + name: "tilde constraint compatible", + cliVersion: "1.0.5", + constraint: "~1.0.0", + compatible: true, + }, + { + name: "tilde constraint incompatible minor bump", + cliVersion: "1.1.0", + constraint: "~1.0.0", + compatible: false, + }, + { + name: "exact version match", + cliVersion: "1.2.3", + constraint: "1.2.3", + compatible: true, + }, + { + name: "exact version mismatch", + cliVersion: "1.2.4", + constraint: "1.2.3", + compatible: false, + }, + { + name: "range constraint compatible", + cliVersion: "1.5.0", + constraint: ">= 1.0.0, < 2.0.0", + compatible: true, + }, + { + name: "range constraint incompatible", + cliVersion: "2.0.0", + constraint: ">= 1.0.0, < 2.0.0", + compatible: false, + }, + // Error cases + { + name: "invalid constraint syntax", + cliVersion: "1.0.0", + constraint: "@invalid", + compatible: false, + expectErr: true, + }, + { + name: "invalid CLI version", + cliVersion: "not-semver", + constraint: ">=1.0.0", + compatible: false, + expectErr: true, + }, } - s.True(names["plugin-one"]) - s.True(names["plugin-two"]) -} - -// TestGetManifest tests manifest retrieval -type ManifestTestSuite struct { - suite.Suite - tempDir string -} - -func TestManifestTestSuite(t *testing.T) { - suite.Run(t, new(ManifestTestSuite)) -} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result, err := compatibleCLIVersion(tt.cliVersion, tt.constraint) -func (s *ManifestTestSuite) SetupTest() { - var err error + if tt.expectErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } - s.tempDir, err = os.MkdirTemp("", "plugin-manifest-test") - s.Require().NoError(err) - - // Reset viper state for each test - viper.Reset() -} - -func (s *ManifestTestSuite) TearDownTest() { - if s.tempDir != "" { - _ = os.RemoveAll(s.tempDir) + assert.Equal(t, tt.compatible, result) + }) } - - viper.Reset() -} - -func (s *ManifestTestSuite) TestGetManifestValid() { - path := createMockPlugin(s.T(), s.tempDir, "dr-test", validManifest) - - manifest, err := getManifest(path) - s.Require().NoError(err) - s.NotNil(manifest) - s.Equal("test-plugin", manifest.Name) - s.Equal("1.0.0", manifest.Version) - s.Equal("Test plugin", manifest.Description) -} - -func (s *ManifestTestSuite) TestGetManifestInvalidJSON() { - path := createMockPlugin(s.T(), s.tempDir, "dr-invalid", invalidManifest) - - manifest, err := getManifest(path) - s.Require().Error(err) - s.Nil(manifest) -} - -func (s *ManifestTestSuite) TestGetManifestNonExistent() { - manifest, err := getManifest(filepath.Join(s.tempDir, "nonexistent")) - s.Require().Error(err) - s.Nil(manifest) -} - -func (s *ManifestTestSuite) TestGetManifestWithConfiguredTimeout() { - // Set a custom timeout via viper - viper.Set("plugin.manifest_timeout_ms", 1000) - - path := createMockPlugin(s.T(), s.tempDir, "dr-test", validManifest) - - manifest, err := getManifest(path) - s.Require().NoError(err) - s.NotNil(manifest) -} - -func (s *ManifestTestSuite) TestGetManifestCommandFailure() { - // Create a script that exits with error - script := `#!/bin/sh -exit 1 -` - path := filepath.Join(s.tempDir, "dr-failing") - s.Require().NoError(os.WriteFile(path, []byte(script), 0o755)) - - manifest, err := getManifest(path) - s.Require().Error(err) - s.Nil(manifest) } From b84279da8ec2b26d634fe9c2446d83f62a1f07f5 Mon Sep 17 00:00:00 2001 From: AJ Alon Date: Fri, 20 Feb 2026 17:42:17 -0800 Subject: [PATCH 3/3] refactor(test): restored deleted discover unit tests, moved CLI versions tests to their own file --- internal/plugin/cli_version_test.go | 173 +++++++++++++ internal/plugin/discover_test.go | 387 +++++++++++++++++----------- 2 files changed, 415 insertions(+), 145 deletions(-) create mode 100644 internal/plugin/cli_version_test.go diff --git a/internal/plugin/cli_version_test.go b/internal/plugin/cli_version_test.go new file mode 100644 index 00000000..17a3a693 --- /dev/null +++ b/internal/plugin/cli_version_test.go @@ -0,0 +1,173 @@ +// Copyright 2026 DataRobot, Inc. and its affiliates. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package plugin + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestCompatibleCLIVersion(t *testing.T) { + tests := []struct { + name string + cliVersion string + constraint string + compatible bool + expectErr bool + }{ + // No constraint - always compatible + { + name: "empty constraint is always compatible", + cliVersion: "1.0.0", + constraint: "", + compatible: true, + }, + // Dev version - always compatible + { + name: "dev CLI version is always compatible", + cliVersion: "dev", + constraint: ">=1.0.0", + compatible: true, + }, + // Latest CLI + plugin with no CLI constraints + { + name: "latest CLI with no constraints", + cliVersion: "2.0.0", + constraint: "", + compatible: true, + }, + // Latest CLI + plugin with CLI min version (using >= constraint) + { + name: "latest CLI satisfies min version constraint", + cliVersion: "2.0.0", + constraint: ">=1.0.0", + compatible: true, + }, + // Latest CLI + plugin with CLI max version (using < constraint) + { + name: "latest CLI within max version constraint", + cliVersion: "1.5.0", + constraint: "< 2.0.0", + compatible: true, + }, + { + name: "latest CLI exceeds max version constraint", + cliVersion: "2.0.0", + constraint: "< 2.0.0", + compatible: false, + }, + // Old CLI + plugin with no CLI constraints + { + name: "old CLI with no constraints", + cliVersion: "0.1.0", + constraint: "", + compatible: true, + }, + // Old CLI + plugin with CLI min version → incompatible + { + name: "old CLI below min version constraint", + cliVersion: "0.1.0", + constraint: ">=1.0.0", + compatible: false, + }, + // Old CLI + plugin with CLI max version + { + name: "old CLI within max version constraint", + cliVersion: "0.1.0", + constraint: "< 2.0.0", + compatible: true, + }, + // Semver constraint patterns + { + name: "caret constraint compatible", + cliVersion: "1.5.0", + constraint: "^1.0.0", + compatible: true, + }, + { + name: "caret constraint incompatible major bump", + cliVersion: "2.0.0", + constraint: "^1.0.0", + compatible: false, + }, + { + name: "tilde constraint compatible", + cliVersion: "1.0.5", + constraint: "~1.0.0", + compatible: true, + }, + { + name: "tilde constraint incompatible minor bump", + cliVersion: "1.1.0", + constraint: "~1.0.0", + compatible: false, + }, + { + name: "exact version match", + cliVersion: "1.2.3", + constraint: "1.2.3", + compatible: true, + }, + { + name: "exact version mismatch", + cliVersion: "1.2.4", + constraint: "1.2.3", + compatible: false, + }, + { + name: "range constraint compatible", + cliVersion: "1.5.0", + constraint: ">= 1.0.0, < 2.0.0", + compatible: true, + }, + { + name: "range constraint incompatible", + cliVersion: "2.0.0", + constraint: ">= 1.0.0, < 2.0.0", + compatible: false, + }, + // Error cases + { + name: "invalid constraint syntax", + cliVersion: "1.0.0", + constraint: "@invalid", + compatible: false, + expectErr: true, + }, + { + name: "invalid CLI version", + cliVersion: "not-semver", + constraint: ">=1.0.0", + compatible: false, + expectErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result, err := compatibleCLIVersion(tt.cliVersion, tt.constraint) + + if tt.expectErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } + + assert.Equal(t, tt.compatible, result) + }) + } +} diff --git a/internal/plugin/discover_test.go b/internal/plugin/discover_test.go index 17a3a693..7da4bb98 100644 --- a/internal/plugin/discover_test.go +++ b/internal/plugin/discover_test.go @@ -15,159 +15,256 @@ package plugin import ( + "fmt" + "os" + "path/filepath" "testing" - "github.com/stretchr/testify/assert" + "github.com/spf13/viper" "github.com/stretchr/testify/require" + "github.com/stretchr/testify/suite" ) -func TestCompatibleCLIVersion(t *testing.T) { - tests := []struct { - name string - cliVersion string - constraint string - compatible bool - expectErr bool - }{ - // No constraint - always compatible - { - name: "empty constraint is always compatible", - cliVersion: "1.0.0", - constraint: "", - compatible: true, - }, - // Dev version - always compatible - { - name: "dev CLI version is always compatible", - cliVersion: "dev", - constraint: ">=1.0.0", - compatible: true, - }, - // Latest CLI + plugin with no CLI constraints - { - name: "latest CLI with no constraints", - cliVersion: "2.0.0", - constraint: "", - compatible: true, - }, - // Latest CLI + plugin with CLI min version (using >= constraint) - { - name: "latest CLI satisfies min version constraint", - cliVersion: "2.0.0", - constraint: ">=1.0.0", - compatible: true, - }, - // Latest CLI + plugin with CLI max version (using < constraint) - { - name: "latest CLI within max version constraint", - cliVersion: "1.5.0", - constraint: "< 2.0.0", - compatible: true, - }, - { - name: "latest CLI exceeds max version constraint", - cliVersion: "2.0.0", - constraint: "< 2.0.0", - compatible: false, - }, - // Old CLI + plugin with no CLI constraints - { - name: "old CLI with no constraints", - cliVersion: "0.1.0", - constraint: "", - compatible: true, - }, - // Old CLI + plugin with CLI min version → incompatible - { - name: "old CLI below min version constraint", - cliVersion: "0.1.0", - constraint: ">=1.0.0", - compatible: false, - }, - // Old CLI + plugin with CLI max version - { - name: "old CLI within max version constraint", - cliVersion: "0.1.0", - constraint: "< 2.0.0", - compatible: true, - }, - // Semver constraint patterns - { - name: "caret constraint compatible", - cliVersion: "1.5.0", - constraint: "^1.0.0", - compatible: true, - }, - { - name: "caret constraint incompatible major bump", - cliVersion: "2.0.0", - constraint: "^1.0.0", - compatible: false, - }, - { - name: "tilde constraint compatible", - cliVersion: "1.0.5", - constraint: "~1.0.0", - compatible: true, - }, - { - name: "tilde constraint incompatible minor bump", - cliVersion: "1.1.0", - constraint: "~1.0.0", - compatible: false, - }, - { - name: "exact version match", - cliVersion: "1.2.3", - constraint: "1.2.3", - compatible: true, - }, - { - name: "exact version mismatch", - cliVersion: "1.2.4", - constraint: "1.2.3", - compatible: false, - }, - { - name: "range constraint compatible", - cliVersion: "1.5.0", - constraint: ">= 1.0.0, < 2.0.0", - compatible: true, - }, - { - name: "range constraint incompatible", - cliVersion: "2.0.0", - constraint: ">= 1.0.0, < 2.0.0", - compatible: false, - }, - // Error cases - { - name: "invalid constraint syntax", - cliVersion: "1.0.0", - constraint: "@invalid", - compatible: false, - expectErr: true, - }, - { - name: "invalid CLI version", - cliVersion: "not-semver", - constraint: ">=1.0.0", - compatible: false, - expectErr: true, - }, +// Test manifests +var ( + validManifest = `{"name":"test-plugin","version":"1.0.0","description":"Test plugin"}` + invalidManifest = `{invalid json` +) + +// createMockPlugin creates a shell script that responds to --dr-plugin-manifest +func createMockPlugin(t *testing.T, dir, name, manifestJSON string) string { + t.Helper() + + script := fmt.Sprintf(`#!/bin/sh +if [ "$1" = "--dr-plugin-manifest" ]; then + echo '%s' +else + exit 0 +fi +`, manifestJSON) + + path := filepath.Join(dir, name) + err := os.WriteFile(path, []byte(script), 0o755) + require.NoError(t, err) + + return path +} + +// DiscoverTestSuite tests discovery functions with filesystem fixtures +type DiscoverTestSuite struct { + suite.Suite + tempDir string +} + +func TestDiscoverTestSuite(t *testing.T) { + suite.Run(t, new(DiscoverTestSuite)) +} + +func (s *DiscoverTestSuite) SetupTest() { + var err error + + s.tempDir, err = os.MkdirTemp("", "plugin-discover-test") + s.Require().NoError(err) +} + +func (s *DiscoverTestSuite) TearDownTest() { + if s.tempDir != "" { + _ = os.RemoveAll(s.tempDir) + } +} + +func (s *DiscoverTestSuite) TestDiscoverInDirEmptyDirectory() { + seen := make(map[string]bool) + plugins, errs := discoverInDir(s.tempDir, seen) + + s.Empty(plugins) + s.Empty(errs) +} + +func (s *DiscoverTestSuite) TestDiscoverInDirNonExistent() { + seen := make(map[string]bool) + plugins, errs := discoverInDir(filepath.Join(s.tempDir, "nonexistent"), seen) + + s.Nil(plugins) + s.Nil(errs) +} + +func (s *DiscoverTestSuite) TestDiscoverInDirValidPlugin() { + createMockPlugin(s.T(), s.tempDir, "dr-testplugin", validManifest) + + seen := make(map[string]bool) + plugins, errs := discoverInDir(s.tempDir, seen) + + s.Require().Len(plugins, 1, "Expected exactly one plugin") + s.Empty(errs) + s.Equal("test-plugin", plugins[0].Manifest.Name) + s.Equal("1.0.0", plugins[0].Manifest.Version) + s.Equal("Test plugin", plugins[0].Manifest.Description) + s.True(seen["test-plugin"]) // seen map now uses manifest.Name +} + +func (s *DiscoverTestSuite) TestDiscoverInDirSkipsNonDrFiles() { + // Create a valid plugin + createMockPlugin(s.T(), s.tempDir, "dr-valid", validManifest) + + // Create non-dr files + s.Require().NoError(os.WriteFile(filepath.Join(s.tempDir, "other-binary"), []byte("#!/bin/sh\nexit 0"), 0o755)) + s.Require().NoError(os.WriteFile(filepath.Join(s.tempDir, "random.txt"), []byte("text"), 0o644)) + + seen := make(map[string]bool) + plugins, _ := discoverInDir(s.tempDir, seen) + + s.Require().Len(plugins, 1, "Expected exactly one plugin (non-dr files should be skipped)") + s.Equal("test-plugin", plugins[0].Manifest.Name) +} + +func (s *DiscoverTestSuite) TestDiscoverInDirSkipsNonExecutable() { + // Create a dr-prefixed file that is not executable + path := filepath.Join(s.tempDir, "dr-notexec") + s.Require().NoError(os.WriteFile(path, []byte("not executable"), 0o644)) + + seen := make(map[string]bool) + plugins, errs := discoverInDir(s.tempDir, seen) + + s.Empty(plugins) + s.Empty(errs) +} + +func (s *DiscoverTestSuite) TestDiscoverInDirHandlesDuplicates() { + createMockPlugin(s.T(), s.tempDir, "dr-duplicate", validManifest) + + // Pre-populate seen map with manifest name (not filename) + seen := map[string]bool{ + "test-plugin": true, // validManifest has name: "test-plugin" + } + + plugins, _ := discoverInDir(s.tempDir, seen) + + // Should be skipped due to duplicate manifest name + s.Empty(plugins) +} + +func (s *DiscoverTestSuite) TestDiscoverInDirDeduplicatesByManifestName() { + // Two different binary names, same manifest name + manifest := `{"name":"shared-name","version":"1.0.0","description":"Test"}` + createMockPlugin(s.T(), s.tempDir, "dr-first", manifest) + createMockPlugin(s.T(), s.tempDir, "dr-second", manifest) + + seen := make(map[string]bool) + plugins, errs := discoverInDir(s.tempDir, seen) + + // Only one should be registered (first one wins based on directory order) + s.Require().Len(plugins, 1, "Expected exactly one plugin when two share the same manifest name") + s.Empty(errs) + s.Equal("shared-name", plugins[0].Manifest.Name) +} + +func (s *DiscoverTestSuite) TestDiscoverInDirInvalidManifest() { + createMockPlugin(s.T(), s.tempDir, "dr-invalid", invalidManifest) + + seen := make(map[string]bool) + plugins, errs := discoverInDir(s.tempDir, seen) + + s.Empty(plugins) + s.Len(errs, 1) // JSON parse error logged +} + +func (s *DiscoverTestSuite) TestDiscoverInDirMultipleValidPlugins() { + manifest1 := `{"name":"plugin-one","version":"1.0.0","description":"First plugin"}` + manifest2 := `{"name":"plugin-two","version":"2.0.0","description":"Second plugin"}` + + createMockPlugin(s.T(), s.tempDir, "dr-one", manifest1) + createMockPlugin(s.T(), s.tempDir, "dr-two", manifest2) + + seen := make(map[string]bool) + plugins, errs := discoverInDir(s.tempDir, seen) + + s.Len(plugins, 2) + s.Empty(errs) + + // Verify both plugins discovered + names := make(map[string]bool) + for _, p := range plugins { + names[p.Manifest.Name] = true } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result, err := compatibleCLIVersion(tt.cliVersion, tt.constraint) + s.True(names["plugin-one"]) + s.True(names["plugin-two"]) +} + +// TestGetManifest tests manifest retrieval +type ManifestTestSuite struct { + suite.Suite + tempDir string +} + +func TestManifestTestSuite(t *testing.T) { + suite.Run(t, new(ManifestTestSuite)) +} - if tt.expectErr { - require.Error(t, err) - } else { - require.NoError(t, err) - } +func (s *ManifestTestSuite) SetupTest() { + var err error - assert.Equal(t, tt.compatible, result) - }) + s.tempDir, err = os.MkdirTemp("", "plugin-manifest-test") + s.Require().NoError(err) + + // Reset viper state for each test + viper.Reset() +} + +func (s *ManifestTestSuite) TearDownTest() { + if s.tempDir != "" { + _ = os.RemoveAll(s.tempDir) } + + viper.Reset() +} + +func (s *ManifestTestSuite) TestGetManifestValid() { + path := createMockPlugin(s.T(), s.tempDir, "dr-test", validManifest) + + manifest, err := getManifest(path) + s.Require().NoError(err) + s.NotNil(manifest) + s.Equal("test-plugin", manifest.Name) + s.Equal("1.0.0", manifest.Version) + s.Equal("Test plugin", manifest.Description) +} + +func (s *ManifestTestSuite) TestGetManifestInvalidJSON() { + path := createMockPlugin(s.T(), s.tempDir, "dr-invalid", invalidManifest) + + manifest, err := getManifest(path) + s.Require().Error(err) + s.Nil(manifest) +} + +func (s *ManifestTestSuite) TestGetManifestNonExistent() { + manifest, err := getManifest(filepath.Join(s.tempDir, "nonexistent")) + s.Require().Error(err) + s.Nil(manifest) +} + +func (s *ManifestTestSuite) TestGetManifestWithConfiguredTimeout() { + // Set a custom timeout via viper + viper.Set("plugin.manifest_timeout_ms", 1000) + + path := createMockPlugin(s.T(), s.tempDir, "dr-test", validManifest) + + manifest, err := getManifest(path) + s.Require().NoError(err) + s.NotNil(manifest) +} + +func (s *ManifestTestSuite) TestGetManifestCommandFailure() { + // Create a script that exits with error + script := `#!/bin/sh +exit 1 +` + path := filepath.Join(s.tempDir, "dr-failing") + s.Require().NoError(os.WriteFile(path, []byte(script), 0o755)) + + manifest, err := getManifest(path) + s.Require().Error(err) + s.Nil(manifest) }