-
Notifications
You must be signed in to change notification settings - Fork 6
feat: add grafanaDependency analyzer #513
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
Open
xnyo
wants to merge
14
commits into
main
Choose a base branch
from
giuseppe/semver-grafana-cloud
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+280
−1
Open
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
975b1da
feat: add cloudversion analyzer
xnyo 5667dea
register cloudversion analyzer
xnyo 67f6363
gen readme
xnyo bbf2aaf
rename to grafanadepepdency
xnyo 1ef3648
fix isGrafanaLabs logic
xnyo 8be3d1f
add IsGrafanaLabs
xnyo c899813
add TestGrafanaDependencyParse
xnyo 651b118
refactor
xnyo 2e35f64
fix getPreRelease
xnyo 7cc8869
add more tests
xnyo 01ef05e
fix getPreRelease tests
xnyo f9f1830
add docstring and more tests
xnyo 2d8495e
update docstring
xnyo 259a6d6
fix integration tests
xnyo File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
80 changes: 80 additions & 0 deletions
80
pkg/analysis/passes/grafanadependency/grafanadependency.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,80 @@ | ||
| package grafanadependency | ||
|
|
||
| import ( | ||
| "encoding/json" | ||
| "fmt" | ||
| "regexp" | ||
|
|
||
| "github.com/grafana/plugin-validator/pkg/analysis" | ||
| "github.com/grafana/plugin-validator/pkg/analysis/passes/metadata" | ||
| ) | ||
|
|
||
| // https://semver.org/#is-there-a-suggested-regular-expression-regex-to-check-a-semver-string | ||
| // Modified to work with semver range operators (>=, >, <, <=) by removing ^ and $ anchors | ||
| var semverRegex = regexp.MustCompile(`(?P<major>0|[1-9]\d*)\.(?P<minor>0|[1-9]\d*)\.(?P<patch>0|[1-9]\d*)(?:-(?P<prerelease>(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\+(?P<buildmetadata>[0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?`) | ||
|
|
||
| var ( | ||
| missingCloudPreRelease = &analysis.Rule{ | ||
| Name: "missing-cloud-pre-release", | ||
| Severity: analysis.Warning, | ||
| } | ||
| ) | ||
|
|
||
| var Analyzer = &analysis.Analyzer{ | ||
| Name: "grafanadependency", | ||
| Requires: []*analysis.Analyzer{metadata.Analyzer}, | ||
| Run: run, | ||
| Rules: []*analysis.Rule{missingCloudPreRelease}, | ||
| ReadmeInfo: analysis.ReadmeInfo{ | ||
| Name: "Grafana Dependency", | ||
| Description: "Ensures the Grafana dependency specified in plugin.json is valid", | ||
| }, | ||
| } | ||
|
|
||
| func run(pass *analysis.Pass) (interface{}, error) { | ||
| metadataBody, ok := pass.ResultOf[metadata.Analyzer].([]byte) | ||
| if !ok { | ||
| return nil, nil | ||
| } | ||
| var data metadata.Metadata | ||
| if err := json.Unmarshal(metadataBody, &data); err != nil { | ||
| return nil, err | ||
| } | ||
| pre := getPreRelease(data.Dependencies.GrafanaDependency) | ||
| if pre == "" && data.IsGrafanaLabs() { | ||
| // Ensure that Grafana Labs plugin specify a pre-release (-99999999999) in Grafana Dependency. | ||
| // If the pre-release part is missing and the grafanaDependency specifies a version that's not | ||
| // been released yet, which is often the case for Grafana Labs plugins and not community/commercial plugins, | ||
| // the plugin won't be loaded correctly in cloud because it doesn't satisfy the Grafana dependency. | ||
| // Example: on a Cloud instance we have Grafana 12.4.0-99999999999. This is a PRE-RELEASE of 12.4.0. | ||
| // If the plugin specifies 12.4.0 as grafanaDependency, it's incompatible with 12.4.0-99999999999. | ||
| // This is because 12.4.0-x (pre-release) < 12.4.0 ("stable") => the plugin can't be installed in Cloud. | ||
| pass.ReportResult( | ||
| pass.AnalyzerName, | ||
| missingCloudPreRelease, | ||
| fmt.Sprintf(`Grafana dependency %q has no pre-release value`, data.Dependencies.GrafanaDependency), | ||
| fmt.Sprintf(`The value of grafanaDependency in plugin.json (%q) is missing a pre-release value. `+ | ||
| `This may make the plugin uninstallable in Grafana Cloud. `+ | ||
| `Please add "-0" as a suffix of your grafanaDependency value ("%s-0")`, | ||
| data.Dependencies.GrafanaDependency, data.Dependencies.GrafanaDependency, | ||
| ), | ||
| ) | ||
| } | ||
| return nil, nil | ||
| } | ||
|
|
||
| // getPreRelease extracts the pre-release identifier from a Grafana dependency version string. | ||
| // It handles semver range operators (>=, >, <, <=) and returns the pre-release part after the dash. | ||
| // Examples: ">=12.4.0-0" returns "0", ">=12.4.0" returns "", ">= 12.4.0-1234" returns "1234". | ||
| func getPreRelease(grafanaDependency string) string { | ||
| matches := semverRegex.FindStringSubmatch(grafanaDependency) | ||
| if matches == nil { | ||
| return "" | ||
| } | ||
| for i, name := range semverRegex.SubexpNames() { | ||
| if name == "prerelease" && i < len(matches) { | ||
| return matches[i] | ||
| } | ||
| } | ||
| return "" | ||
| } | ||
136 changes: 136 additions & 0 deletions
136
pkg/analysis/passes/grafanadependency/grafanadependency_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,136 @@ | ||
| package grafanadependency | ||
|
|
||
| import ( | ||
| "encoding/json" | ||
| "fmt" | ||
| "path/filepath" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
|
|
||
| "github.com/grafana/plugin-validator/pkg/analysis" | ||
| "github.com/grafana/plugin-validator/pkg/analysis/passes/archive" | ||
| "github.com/grafana/plugin-validator/pkg/analysis/passes/metadata" | ||
| "github.com/grafana/plugin-validator/pkg/analysis/passes/metadatavalid" | ||
| "github.com/grafana/plugin-validator/pkg/testpassinterceptor" | ||
| ) | ||
|
|
||
| func TestGrafanaDependency(t *testing.T) { | ||
| for _, tc := range []struct { | ||
| name string | ||
| pluginJSON string | ||
| expFlag bool | ||
| }{ | ||
| { | ||
| name: "non-grafana labs plugin without pre-release shouldn't be flagged", | ||
| pluginJSON: `{ | ||
| "id": "community-my-app", | ||
| "dependencies": { "grafanaDependency": ">=11.6.0" } | ||
| }`, | ||
| expFlag: false, | ||
| }, | ||
| { | ||
| name: "non-grafana labs plugin with pre-release shouldn't be flagged", | ||
| pluginJSON: `{ | ||
| "id": "community-my-app", | ||
| "dependencies": { "grafanaDependency": ">=11.6.0-0" } | ||
| }`, | ||
| expFlag: false, | ||
| }, | ||
| { | ||
| name: "grafana org plugin without pre-release should be flagged", | ||
| pluginJSON: `{ | ||
| "id": "grafana-my-app", | ||
| "dependencies": { "grafanaDependency": ">=11.6.0" } | ||
| }`, | ||
| expFlag: true, | ||
| }, | ||
| { | ||
| name: "grafana labs author plugin without pre-release should be flagged", | ||
| pluginJSON: `{ | ||
| "id": "adopted-my-app", | ||
| "info": { "author": { "name": "Grafana Labs" } }, | ||
| "dependencies": { "grafanaDependency": ">=11.6.0" } | ||
| }`, | ||
| expFlag: true, | ||
| }, | ||
| { | ||
| name: "grafana org plugin with pre-release should not be flagged", | ||
| pluginJSON: `{ | ||
| "id": "grafana-my-app", | ||
| "dependencies": { "grafanaDependency": ">= 11.6.0-0" } | ||
| }`, | ||
| expFlag: false, | ||
| }, | ||
| { | ||
| name: "grafana labs author plugin with pre-release should not be flagged", | ||
| pluginJSON: `{ | ||
| "id": "adopted-my-app", | ||
| "info": { "author": { "name": "Grafana Labs" } }, | ||
| "dependencies": { "grafanaDependency": ">= 11.6.0-0" } | ||
| }`, | ||
| expFlag: false, | ||
| }, | ||
| } { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| var interceptor testpassinterceptor.TestPassInterceptor | ||
| pass := &analysis.Pass{ | ||
| RootDir: filepath.Join("./"), | ||
| ResultOf: map[*analysis.Analyzer]interface{}{ | ||
| metadata.Analyzer: []byte(tc.pluginJSON), | ||
| archive.Analyzer: filepath.Join("."), | ||
| metadatavalid.Analyzer: nil, | ||
| }, | ||
| Report: interceptor.ReportInterceptor(), | ||
| } | ||
|
|
||
| _, err := Analyzer.Run(pass) | ||
| require.NoError(t, err) | ||
| if tc.expFlag { | ||
| require.Len(t, interceptor.Diagnostics, 1) | ||
|
|
||
| var meta metadata.Metadata | ||
| require.NoError(t, json.Unmarshal(pass.ResultOf[metadata.Analyzer].([]byte), &meta)) | ||
| grafanaDependency := meta.Dependencies.GrafanaDependency | ||
| require.NotEmpty(t, grafanaDependency, "grafana dependency should not be empty") | ||
|
|
||
| d := interceptor.Diagnostics[0] | ||
| require.Equal(t, analysis.Warning, d.Severity, "severity should be warning") | ||
| require.Equal(t, "missing-cloud-pre-release", d.Name, "name should match") | ||
| require.Equal(t, fmt.Sprintf(`Grafana dependency "%s" has no pre-release value`, grafanaDependency), d.Title, "title should match") | ||
| require.Equal(t, fmt.Sprintf(`The value of grafanaDependency in plugin.json ("%s") is missing a pre-release value. This may make the plugin uninstallable in Grafana Cloud. Please add "-0" as a suffix of your grafanaDependency value ("%s-0")`, grafanaDependency, grafanaDependency), d.Detail, "detail should match") | ||
| } else { | ||
| ok := assert.Len(t, interceptor.Diagnostics, 0, "expecting no diagnostics but got %d", len(interceptor.Diagnostics)) | ||
| // Log for debugging | ||
| if !ok { | ||
| for _, d := range interceptor.Diagnostics { | ||
| t.Logf("%+v", d) | ||
| } | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestGrafanaDependency_GetPreRelease(t *testing.T) { | ||
| for _, tc := range []struct { | ||
| name string | ||
| dependency string | ||
| expPre string | ||
| }{ | ||
| {"no pre-release", ">=12.4.0", ""}, | ||
| {"no pre-release with space", ">= 12.4.0", ""}, | ||
| {"zero pre-release", ">=12.4.0-0", "0"}, | ||
| {"zero pre-release with space", ">= 12.4.0-0", "0"}, | ||
| {"non-zero pre-release", ">=12.4.0-1189998819991197253", "1189998819991197253"}, | ||
| {"non-zero pre-release with space", ">= 12.4.0-1189998819991197253", "1189998819991197253"}, | ||
| {"non-numeric pre-release", ">=12.4.0-alpha.1", "alpha.1"}, | ||
| {"non-numeric pre-release with space", ">= 12.4.0-alpha-2", "alpha-2"}, | ||
| } { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| pre := getPreRelease(tc.dependency) | ||
| require.Equal(t, tc.expPre, pre, "extracted pre-release value should match") | ||
| }) | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
What do you think of this regex https://github.com/grafana/grafana/pull/118090/changes?
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.
isn't it better to use a semver library for this? there are some, more complex ranges out there like this
Uh oh!
There was an error while loading. Please reload this page.
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.
If we were to validate with semver lib, shouldn't we include it in the metadatavalid check? Remove the
grafanaDependencyobject from the schema so schema validation passes and validate the actual value ofgrafanaDependencywith SemVer lib.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.
@s4kh Regex is good, but the current code added in this PR won't parse those correctly.
@andresmgot I tried using the semver lib we use on Grafana's backend (https://github.com/Masterminds/semver) but there are no ways to parse a semver range and inspect its content (to ensure that the pre-release is part of the constraint) with that library. It looks like we can just validate if a version satisfies a semver range or not.
I just noticed that plugin-validator uses https://github.com/hashicorp/go-version instead: this one exports a
Constraint.Prereleasefunction that might be what we need: https://pkg.go.dev/github.com/hashicorp/go-version#Constraint.Prerelease but it's a bit weird because there may be more than one semver version in the constraint and I'm not sure how the function behaves in such case. I'll take a look