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

Validate instance version before SQL validation #87

Merged
merged 2 commits into from Oct 31, 2019

Conversation

@joshtemple
Copy link
Collaborator

joshtemple commented Oct 30, 2019

Closes #81.

@joshtemple joshtemple force-pushed the feature/instance-version branch from 182474a to 9315570 Oct 31, 2019
@joshtemple joshtemple requested a review from DylanBaker Oct 31, 2019
@codecov

This comment has been minimized.

Copy link

codecov bot commented Oct 31, 2019

Codecov Report

Merging #87 into master will decrease coverage by 2.67%.
The diff coverage is 18.51%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #87      +/-   ##
=========================================
- Coverage   75.27%   72.6%   -2.68%     
=========================================
  Files           9       9              
  Lines         546     573      +27     
=========================================
+ Hits          411     416       +5     
- Misses        135     157      +22
Impacted Files Coverage Δ
spectacles/validators.py 63.88% <75%> (+0.31%) ⬆️
spectacles/client.py 65.48% <8.69%> (-14.52%) ⬇️

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 f9ae84c...9315570. Read the comment docs.

@joshtemple joshtemple merged commit 5449100 into master Oct 31, 2019
1 of 3 checks passed
1 of 3 checks passed
codecov/patch 18.51% of diff hit (target 75.27%)
Details
codecov/project 72.6% (-2.68%) compared to f9ae84c
Details
ci/circleci: test Your tests passed on CircleCI!
Details
@@ -43,9 +43,18 @@ class SqlValidator(Validator):
"""

timeout = aiohttp.ClientTimeout(total=300)
MIN_LOOKER_VERSION = "6.22.12"

This comment has been minimized.

Copy link
@agile

agile Nov 7, 2019

Just wanting to confirm this is in fact the min version required, because #81 mentions 6.20.12?

This comment has been minimized.

Copy link
@joshtemple

joshtemple Nov 7, 2019

Author Collaborator

Looks like the version in #81 is a typo (I've fixed it). However, we were told by Looker
that the API fix required here should be backported to 6.20, but I’m not
sure if that’s happened yet.

Anything after 6.22.12 should have the fix we’re checking for here, but if
the back port has occurred it should work on 6.20+

If you are able to get it working on pre-6.22.12, please let us know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.