dapr / components-contrib

Community driven, reusable components for distributed apps
Apache License 2.0
544 stars 473 forks source link

Automatic validation of metatada.yaml contents against its schema #2780

Closed tmacam closed 1 year ago

tmacam commented 1 year ago

Describe the feature

During the build process we should validate if the many metadata.yaml validate against its schema to avoid PRs like #2777.

We already do some validate (for missing metadata fields) as part of the build process (introduced by #2696) but we do not validate if the contents of those YAML files all validate against the schema. For instance, #2777 fixes an issue that a given YAML file had numeric values for its default and example fields -- and, as the PR title says, "even numbers must have string values for default and example fields. Right now we don't have any automatic step to prevent such errors.

There are a few ways we could take to address this:

  1. Augument make check-component-metadata to also perform schema validation.
  2. Add make bundle-component-metadata as one part of one GH validation workflows. This target performs some validation on the YAML contents, so it could be an easy addition.
  3. Building on the last one, take the JSON bundle and validate the resulting JSON against the JSON schema defined in component-metadata-schema.json

Release Note

RELEASE NOTE: ADD Automatic validation of metatada.yaml contents against its schema.

ItalyPaleAle commented 1 year ago

Thanks for writing this up. Let's go with #1 IMHO, I like the idea of augmenting our static analysis tools, there can be other nice things we can do with that in the future.

Since the schema is auto-generated from the Go structs in the build tools: https://vscode.dev/github/ItalyPaleAle/dapr-components-contrib/blob/26dea0c1c20e7f3a05a5c625d82abb201a1504cc/.build-tools/pkg/metadataschema/schema.go We should be able to just validate that we can un-marshal into the JSON struct.

This may also be a smart way to catch things like YAML's famous "Norway problem"

tmacam commented 1 year ago

Yeah. That definition uses json tags but it seems we can pre-process those structures and make them more YAML-friendly (see the gist at the very end of https://github.com/go-yaml/yaml/issues/424)

Just using UnmarshalStrict uncovers quite a bit.

--- a/.build-tools/pkg/metadataanalyzer/analyzer.template
+++ b/.build-tools/pkg/metadataanalyzer/analyzer.template
@@ -95,7 +95,7 @@ func getYamlMetadata(basePath string, pkg string) *map[string]string {
        }

        var d Data
-       err = yaml.Unmarshal(data, &d)
+       err = yaml.UnmarshalStrict(data, &d)
        if err != nil {
                fmt.Println(fmt.Errorf("Invalid metadata yaml format. Error unmarshalling yaml %s: %s", metadatayamlpath, err.Error()))
                os.Exit(1)

We might want to have a 2 pass strategy:

  1. quickly identify missing keys (current behaviour) and
  2. enforce agreement to schema.
github-actions[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged (pinned, good first issue, help wanted or triaged/resolved) or other activity occurs. Thank you for your contributions.

tmacam commented 1 year ago

Adding another validation that we could do (and to keep this issue open): ensure that properties biding declarations don't conflict with the component bindings declaration. For instance, it makes no sense to define an output-only propety for an input-only binding component. That sort of validation should/could also be performed.

github-actions[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged (pinned, good first issue, help wanted or triaged/resolved) or other activity occurs. Thank you for your contributions.

github-actions[bot] commented 1 year ago

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as pinned, good first issue, help wanted or triaged/resolved. Thank you for your contributions.