Skip to content
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

Fix model validation, add CI #16

Merged
merged 14 commits into from
Sep 19, 2022
Merged

Fix model validation, add CI #16

merged 14 commits into from
Sep 19, 2022

Conversation

kochalex
Copy link
Contributor

@kochalex kochalex commented Sep 13, 2022

When trying to use these schemas in a CI workflow with our DBT project, I ran into a few errors with the JSON schema documents with both Ajv and jsonschema.

This has a couple of fixes that allowed the schemas themselves to be validated as draft 7. (I used draft 7 since it's the most common from my experience, but it might also be good to add a $schema entry to the files). There's also a sample GitHub Actions CI workflow that tests using ajv-cli:

  1. If the schema files pass draft7
  2. Testing simple dbt_project.yml and model.yml files generated with dbt init.

Here's a run on my fork: https://github.com/kochalex/dbt-jsonschema/actions/runs/3048375813

Screen Shot 2022-09-13 at 4 22 48 PM

Separately, are there any plans for DBT to publish these schemas in a registry like https://schemas.getdbt.com/ or have them officially part of DBT?

I've pointed my VS Code at these updated files and our project's YAML files validate ok, but it should be noted that the existing files also seemed to work ok with the VS Code YAML extension, it was just the CI cases that failed (with two separate validators). I know CI is not the intended use case here. 😸

kochalex and others added 10 commits September 13, 2022 14:25
* make column_properties/required an arrary

* escape regex

* fix cli for installing ajv-cli

* escape regexes in the other files, get them passing validation

* add node_modules to gitignore

* update npm cli to not write package.json & lock file

* ci job testing against real files

* remove cache key from jobs, not using package.json

* job is using fixture files as a test, not running dbt init
@@ -131,7 +131,7 @@
"oneOf": [
{
"type": "string",
"pattern": "{{.*}}"
"pattern": "\\{\\{.*\\}\\}"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

@joellabes joellabes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome @kochalex! Thanks for doing this.

I'm happy with schema version 7 if that's the default - I skipped on picking a version because I wasn't sure what the right one was, and it seemed to work without specifying one 😬 if you want to add that to these files as well I'd be happy to include it in this PR?

Otherwise, just two pedantic changes around capitalisation (or lack thereof) of dbt.

Such a good idea to put some CI on this, json is fiddly at the best of times.

tests/dbt_project.yml Outdated Show resolved Hide resolved
tests/schema.yml Outdated Show resolved Hide resolved
@joellabes
Copy link
Collaborator

Separately, are there any plans for DBT to publish these schemas in a registry like https://schemas.getdbt.com/ or have them officially part of DBT?

Not yet, but over time as they get more robust it's not out of the question! We're also thinking about listing on schemastore, but that has some tradeoffs (see #3)

@kochalex
Copy link
Contributor Author

Ok, I've made the changes (caps and adding in $schema), would you like me to cleanup & squash the history on this?

@joellabes
Copy link
Collaborator

@kochalex we squash and merge so it's all good! Thank you for doing this 🌟

@joellabes joellabes merged commit bcaa460 into dbt-labs:main Sep 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants