-
Notifications
You must be signed in to change notification settings - Fork 13
[CFX-4730] Add CLI version constraints to plugin manifest #357
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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) | ||
| }) | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test regex rejects valid semver constraint syntaxMedium Severity The validation regex |
||
| } | ||
| }) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
Comment on lines
42
to
+44
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is backwards incompatible, but I'm not seeing an update in the plugin that would be emitting the wrong version. So either we should support both names (good ole backwards compat), or at least fix the assist plugin in this PR to use the new one with a new version. |
||
| } | ||
|
|
||
| // RegistryVersion represents a specific version in the plugin registry | ||
|
|
||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parse error in version check silently loads plugin
Medium Severity
When
compatibleCLIVersionreturns an error (e.g., malformed constraint like@invalid), theif err != nilbranch only logs a warning and falls through, so the plugin is still loaded. Theelse if !compatiblebranch that skips the plugin is never reached whenerr != nil. This meanscompatibleCLIVersionreturnsfalseon error, but the callers ignore thatfalsevalue and load the plugin anyway. BothloadManagedPluginanddiscoverInDirhave this same pattern, making constraint enforcement ineffective for malformed constraints.Additional Locations (1)
internal/plugin/discover.go#L273-L287