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

Add content validator #236

Merged
merged 33 commits into from
Jun 23, 2020
Merged

Add content validator #236

merged 33 commits into from
Jun 23, 2020

Conversation

joshtemple
Copy link
Collaborator

@joshtemple joshtemple commented Jun 16, 2020

Closes #138.

Adds a validator for content validation spectacles content with the following validator-specific parameters:

  • --incremental: ignore errors that are already present in production
  • --exclude-personal: ignore errors in content in personal folders

There are a couple other notable differences from the Looker content validator:

  • Can use --explores and --exclude to test specific LookML for content issues
  • Only returns errors for the project specified by --project instead of for the entire instance

Misc changes also included in this PR:

  • Validator module broken up into 3 validator-specific module and moved to its own subpackage
  • Print help message on query mode in dim text
  • Suggest that the project name is wrong if error encountered getting active Git branch
  • LookML objects now have a list of errors instead of a single error
  • log_duration moved to the CLI functions instead of the Runner methods
  • Selector parsing moved to an independent module and now uses regex for easier code
  • Validators now print the first 6 chars of the commit or the branch reference being validated against

@joshtemple
Copy link
Collaborator Author

This PR needs tests before it's ready for review.

@codecov
Copy link

codecov bot commented Jun 18, 2020

Codecov Report

Merging #236 into master will increase coverage by 0.12%.
The diff coverage is 86.52%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #236      +/-   ##
==========================================
+ Coverage   89.71%   89.84%   +0.12%     
==========================================
  Files          13       17       +4     
  Lines        1060     1241     +181     
==========================================
+ Hits          951     1115     +164     
- Misses        109      126      +17     
Impacted Files Coverage Δ
spectacles/cli.py 74.58% <50.00%> (-4.13%) ⬇️
spectacles/runner.py 74.41% <66.66%> (-7.64%) ⬇️
spectacles/validators/content.py 91.93% <91.93%> (ø)
spectacles/lookml.py 87.29% <97.29%> (+5.85%) ⬆️
spectacles/client.py 98.74% <100.00%> (+0.10%) ⬆️
spectacles/exceptions.py 93.87% <100.00%> (+0.54%) ⬆️
spectacles/printer.py 95.08% <100.00%> (+18.29%) ⬆️
spectacles/utils.py 85.71% <100.00%> (+0.34%) ⬆️
spectacles/validators/__init__.py 100.00% <100.00%> (ø)
spectacles/validators/data_test.py 100.00% <100.00%> (ø)
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 45a0b42...07d71dd. Read the comment docs.

@joshtemple joshtemple force-pushed the feature/content-validation branch from 9d4bc83 to 1d52909 Compare June 20, 2020 18:50
Use tmpdir for SQL logging

Try creating parents too

Mock logging call
@joshtemple joshtemple force-pushed the feature/content-validation branch from ef1ee23 to 07a80ab Compare June 20, 2020 19:35
@joshtemple joshtemple marked this pull request as ready for review June 20, 2020 21:39
@joshtemple joshtemple requested a review from DylanBaker as a code owner June 20, 2020 21:39
@joshtemple
Copy link
Collaborator Author

@DylanBaker, there are a few limitations to this validator in the current state:

  1. If an explore is deleted, we won't be able to associate the content error to the Project because the errors returned by the API are not project-qualified. It is possible to fix this and display dangling errors (Return dangling content validation errors #240) but it will require additional dev because currently we prune the Project to only the models and explores that have been selected.
  2. For the same reason, if there are two projects with the same model with the same explore in each, we could appropriate the error to the wrong project. This is a Looker API limitation, to my knowledge there is no way around this at the moment.

@DylanBaker
Copy link
Collaborator

@joshtemple Thanks. That makes sense. It sounds like we could solve both those issues if we could convince Looker to return the project name from that endpoint?

@joshtemple joshtemple force-pushed the feature/content-validation branch from 0fdf28b to 07d71dd Compare June 22, 2020 15:46
@joshtemple
Copy link
Collaborator Author

That's correct. I've created #240 and #241 to track this missing functionality and I'll make sure it's in the docs PR as caveats as well.

@joshtemple joshtemple merged commit 95b0f85 into master Jun 23, 2020
@joshtemple joshtemple deleted the feature/content-validation branch September 1, 2021 18:51
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.

Add a content validator
2 participants