feat: validate grafanaDep with semver by s4kh · Pull Request #515 · grafana/plugin-validator

s4kh · 2026-02-16T15:26:02Z

Validate plugin.json -> dependencies.grafanaDependency using SemVer constraint.
Ignores schema validation for dependencies.grafanaDependency.

s4kh · 2026-02-16T15:30:36Z

pkg/analysis/passes/metadatavalid/metadatavalid.go

 		break
 	}
+
+	pluginJsonBytes, _ := os.ReadFile(metadataPath)


At this stage, we know plugin.json file exists so no need to check for that.

What if the file fails to open or to be read? I'd still keep the error check just in case

s4kh · 2026-02-16T15:31:55Z

pkg/analysis/passes/metadatavalid/metadatavalid.go

 	for _, desc := range result.Errors() {
+		// we validate grafanaDependency at line 91-100,
+		// so we ignore the error from schema validation
+		if strings.Contains(desc.Field(), "grafanaDependency") {


I tried removing from the schema, but we have to read the schema and iterate through and find the grafanaDependency object and remove. Ignoring the returned error seemed simple so went with it.

academo · 2026-02-16T16:04:35Z

pkg/analysis/passes/metadatavalid/metadatavalid.go

+		if err := json.Unmarshal(pluginJsonBytes, &data); err == nil {
+			fmt.Println(data.Dependencies.GrafanaDependency)
+			_, err = version.NewConstraint(data.Dependencies.GrafanaDependency)
+			fmt.Println(err)


remove this

academo · 2026-02-16T16:04:48Z

pkg/analysis/passes/metadatavalid/metadatavalid.go

+	if pluginJsonBytes != nil {
+		var data metadata.Metadata
+		if err := json.Unmarshal(pluginJsonBytes, &data); err == nil {
+			fmt.Println(data.Dependencies.GrafanaDependency)


remove print

academo · 2026-02-16T16:06:23Z

pkg/analysis/passes/metadatavalid/metadatavalid_test.go

+	_, err := Analyzer.Run(pass)
+	require.NoError(t, err)
+	require.Len(t, interceptor.Diagnostics, 2)
+	require.Equal(


nit: validate that the schema is not reporting grafanaDependency as it used to be.

now the new error message is checked in the main_test.go

academo · 2026-02-16T16:06:33Z

pkg/analysis/passes/metadatavalid/metadatavalid_test.go

+	)
+	require.Equal(
+		t,
+		"plugin.json: (root): Additional property invalid is not allowed",


do we really need this validation here?

It is to check combination of schema and grafanaDependency.

pkg/analysis/passes/metadatavalid/testdata/invalid/grafana-dep/plugin.json

xnyo

Some small comments but the rest LGTM to me. I don't think this will conflict with #513 as they currently do two different things, but since the analyzer in #513 is called grafanaDependency we can maybe move this check over there instead once merged? (before cutting a release because the analyzer name will change if we move a check from one analyzer to another)

xnyo · 2026-02-16T16:17:26Z

pkg/analysis/passes/metadatavalid/metadatavalid.go

 		break
 	}
+
+	pluginJsonBytes, _ := os.ReadFile(metadataPath)


What if the file fails to open or to be read? I'd still keep the error check just in case

pkg/analysis/passes/metadatavalid/metadatavalid.go

xnyo

I went through this again considering also #513. I think we should merge this PR first, but we should make some changes before merging. My proposal is the following:

Move the grafanaDependency check via the semver library in a separate grafanadependency analyzer, similar to #513. I would be fine with keeping the semver range check in the metadatavalid analyzer, but we need the extra checks in #513 (mostly just for our internal teams as sometimes they tend to forget the pre-release constraint -> deploy to cloud -> cause an incident because the version is "not supported". It never happens with community plugins since pretty much nobody uses on-prem "rolling" releases off main), so I think a dedicated analyzer is more suited, to keep those grafanaDependency-related checks close to each other

Since the proposal is to move those more robust checks with the semver library in a different analyzer, I think we should remove the grafanaDependency error filtering done here:

plugin-validator/pkg/analysis/passes/metadatavalid/metadatavalid.go

Lines 122 to 127 in bbbfb76

    
           // we validate grafanaDependency at line 91-100, 
        
           // so we ignore the error from schema validation 
        
           if strings.Contains(desc.Field(), "grafanaDependency") || strings.Contains(desc.Description(), "grafanaDependency") { 
        
           	errLen -= 1 
        
           	continue 
        
           }

, unless it will conflict with the semver library check. I don't know if they will conflict with each other, but ideally they shouldn't: I see the regex as a first level of validation (weaker), and the semver library as a second one (more robust). If for some reason there are instances where the regex/schema check fails, but it should pass when fed into the semver library, I think we should change the regex instead of filtering out the error. If we decide to move the semver check into a dedicated analyzer, the filtering done in metadatavalid (like in this PR) will become messy because it would reference checks done on a separate grafanadependency analyzer. Moreover, if the user uses a config file that disables grafanadependency but keeps metadatavalid, the checks on the grafanaDependency field are skipped completely. I'd still keep a "simpler" check in metadatavalid and a proper one in a brand new analyzer

Once we merge this PR, I will rework #513 also considering your previous feedback (this PR is in a better state than #513, we're better off unblocking this one first)

What are your thoughts on this? @s4kh @academo

s4kh · 2026-02-17T17:21:50Z

I think the main problem with validating grafanaDependency in the metadatavalid check is the complexity of regex. Semver constraint can get pretty complex, and we need to adjust the regex every time - or sometimes regex fails but the other dedicated check passes or vice versa. And since we will be already using the semver validator for the grafanaDependency do we need to validate in the metadatavalid? I'll move it to the separate analyzer.

s4kh · 2026-02-17T19:52:14Z

pkg/analysis/passes/grafanadependency/grafanadependency_test.go

+			name: "complex but valid grafanaDependency constraint",
+			pluginJSON: `{
+				"id": "test-org-app",
+				"dependencies": { "grafanaDependency": ">=11.6.11 <12 || >=12.0.10 <12.1 || >=12.1.7 <12.2 || >=12.2.5" }


Using github.com/Masterminds/semver/v3 v3.4.0 cause hashicorp/go-version treats this as invalid. Grafana treats as valid -> grafana/grafana#118090

xnyo

Just one small comment, the rest LGTM! Great job! 🙌

xnyo · 2026-02-19T15:57:15Z

pkg/analysis/passes/grafanadependency/grafanadependency_test.go

+)
+
+func TestGrafanaDependency(t *testing.T) {
+	for _, tc := range []struct {


Can you add test cases for:

Empty grafanaDependency

Missing grafanaDependency (if that makes sense, since there's also the schema validation)

Got it. Added the tests. Unmarshal succeeds and NewConstraint returns error - improper constraint thus same title message is returned

* feat: validate grafanaDep with semver * test: add parent key * test: error msg change * ref: log the error * ref: extract to separate analyzer, use semver * test: empty or missing grafana dep * chore: retrigger ci

s4kh requested review from a team as code owners February 16, 2026 15:26

s4kh requested review from andresmgot, jackw, oshirohugo and xnyo February 16, 2026 15:26

github-project-automation bot added this to Grafana Catalog Team Feb 16, 2026

github-project-automation bot moved this to 📬 Triage in Grafana Catalog Team Feb 16, 2026

s4kh commented Feb 16, 2026

View reviewed changes

s4kh self-assigned this Feb 16, 2026

s4kh requested a review from academo February 16, 2026 15:32

academo reviewed Feb 16, 2026

View reviewed changes

pkg/analysis/passes/metadatavalid/testdata/invalid/grafana-dep/plugin.json Outdated Show resolved Hide resolved

grafana-plugins-platform-bot bot moved this from 📬 Triage to 🔬 In review in Grafana Catalog Team Feb 16, 2026

s4kh force-pushed the feat-validate-semver branch from a3f0e0a to 4734a26 Compare February 16, 2026 16:20

xnyo reviewed Feb 16, 2026

View reviewed changes

s4kh force-pushed the feat-validate-semver branch from 4734a26 to ca6cf1b Compare February 16, 2026 16:36

s4kh requested review from academo and xnyo February 16, 2026 17:04

xnyo reviewed Feb 17, 2026

View reviewed changes

s4kh force-pushed the feat-validate-semver branch from cfe2d0a to 79ae153 Compare February 17, 2026 19:49

s4kh commented Feb 17, 2026

View reviewed changes

s4kh force-pushed the feat-validate-semver branch 2 times, most recently from 6fca874 to 71f0efe Compare February 17, 2026 20:29

s4kh requested a review from xnyo February 18, 2026 14:43

s4kh added 5 commits February 19, 2026 09:31

feat: validate grafanaDep with semver

8584ff7

test: add parent key

3d58842

test: error msg change

ebfb600

ref: log the error

efc922e

ref: extract to separate analyzer, use semver

309ccac

s4kh force-pushed the feat-validate-semver branch from 71f0efe to 309ccac Compare February 19, 2026 14:31

xnyo approved these changes Feb 19, 2026

View reviewed changes

s4kh added 2 commits February 19, 2026 11:11

test: empty or missing grafana dep

cfb934f

chore: retrigger ci

0ce9e9a

s4kh merged commit b0f0069 into main Feb 20, 2026
8 checks passed

s4kh deleted the feat-validate-semver branch February 20, 2026 14:07

github-project-automation bot moved this from 🔬 In review to 🚀 Shipped in Grafana Catalog Team Feb 20, 2026

	// we validate grafanaDependency at line 91-100,
	// so we ignore the error from schema validation
	if strings.Contains(desc.Field(), "grafanaDependency") \|\| strings.Contains(desc.Description(), "grafanaDependency") {
	errLen -= 1
	continue
	}

Comments

Conversation

s4kh commented Feb 16, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xnyo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xnyo left a comment

Choose a reason for hiding this comment

Uh oh!

s4kh commented Feb 17, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xnyo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants