Conversation
| break | ||
| } | ||
|
|
||
| pluginJsonBytes, _ := os.ReadFile(metadataPath) |
There was a problem hiding this comment.
At this stage, we know plugin.json file exists so no need to check for that.
There was a problem hiding this comment.
What if the file fails to open or to be read? I'd still keep the error check just in case
| 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") { |
There was a problem hiding this comment.
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.
| if err := json.Unmarshal(pluginJsonBytes, &data); err == nil { | ||
| fmt.Println(data.Dependencies.GrafanaDependency) | ||
| _, err = version.NewConstraint(data.Dependencies.GrafanaDependency) | ||
| fmt.Println(err) |
| if pluginJsonBytes != nil { | ||
| var data metadata.Metadata | ||
| if err := json.Unmarshal(pluginJsonBytes, &data); err == nil { | ||
| fmt.Println(data.Dependencies.GrafanaDependency) |
| _, err := Analyzer.Run(pass) | ||
| require.NoError(t, err) | ||
| require.Len(t, interceptor.Diagnostics, 2) | ||
| require.Equal( |
There was a problem hiding this comment.
nit: validate that the schema is not reporting grafanaDependency as it used to be.
There was a problem hiding this comment.
now the new error message is checked in the main_test.go
| ) | ||
| require.Equal( | ||
| t, | ||
| "plugin.json: (root): Additional property invalid is not allowed", |
There was a problem hiding this comment.
do we really need this validation here?
There was a problem hiding this comment.
It is to check combination of schema and grafanaDependency.
| "name": "Clock", | ||
| "id": "grafana-clock-panel", | ||
| "skipDataQuery": true, | ||
| "invalid": "invalid", |
There was a problem hiding this comment.
we can remove this, we want to test the grafanaDependency, there are tests already for invalid schema fields.
grafana-plugins-platform-bot
bot
moved this from 📬 Triage
to 🔬 In review
in Grafana Catalog Team
s4kh
force-pushed
the
feat-validate-semver
branch
from
a3f0e0a to
4734a26
Compare
xnyo
left a comment
xnyo
left a comment
There was a problem hiding this comment.
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)
| break | ||
| } | ||
|
|
||
| pluginJsonBytes, _ := os.ReadFile(metadataPath) |
There was a problem hiding this comment.
What if the file fails to open or to be read? I'd still keep the error check just in case
| if pluginJsonBytes != nil { | ||
| var data metadata.Metadata | ||
| if err := json.Unmarshal(pluginJsonBytes, &data); err == nil { | ||
| _, err = version.NewConstraint(data.Dependencies.GrafanaDependency) |
There was a problem hiding this comment.
Just to confirm: with this change we will validate both that the GrafanaDependency is valid according to the regex in the schema + actual semver library used by Grafana's backend?
There was a problem hiding this comment.
It will use hashicorp/semver to validate the constraint. Schema validation for grafanaDependency will be ignored.
s4kh
force-pushed
the
feat-validate-semver
branch
from
4734a26 to
ca6cf1b
Compare
3 participants
Validate plugin.json ->
dependencies.grafanaDependencyusing SemVer constraint.Ignores schema validation for
dependencies.grafanaDependency.