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

Be less restrictive with Looker version check #118

Merged

Conversation

agile
Copy link
Contributor

@agile agile commented Nov 12, 2019

If the version detected is less than the defined minimum, issue a warning noting the version being used is not supported rather than refusing to run at all. (maybe it should also warn that it may crash Looker?)

We are running an older version of Looker (6.16.x) and I wanted to see if it simply would not work at all or if we were able to at least gain some utility. To my surprise, it seems to work ok, I have not run into the 500 errors (#73) that led to #81, yet.

@codecov
Copy link

codecov bot commented Nov 12, 2019

Codecov Report

Merging #118 into master will increase coverage by 1%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #118   +/-   ##
=======================================
+ Coverage   65.46%   66.47%   +1%     
=======================================
  Files           9        9           
  Lines         724      683   -41     
=======================================
- Hits          474      454   -20     
+ Misses        250      229   -21
Impacted Files Coverage Δ
spectacles/client.py 56.71% <ø> (+4.68%) ⬆️
spectacles/validators.py 59.3% <ø> (-1.51%) ⬇️

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 a21092c...2730da9. Read the comment docs.

@DylanBaker
Copy link
Collaborator

@agile Are you on self-hosted?

The regression that needed #81 was introduced in 6.20 AFAIK, which makes sense that you are able to run on a previous version and makes a good argument for loosening this.

@agile
Copy link
Contributor Author

agile commented Nov 13, 2019

Yes, self hosted. Good to know that it's something introduced in 6.20, I believe we've been planning to upgrade to that fairly soon but maybe we can leap frog up to 6.22 or better instead.

@joshtemple
Copy link
Collaborator

The API fix should've been backported to 6.20, so it's possible it may even work for 6.20 instances. I'm comfortable removing this restriction altogether (including the warning proposed here) until we hear otherwise from people on 6.20-6.22.

@DylanBaker, thoughts?

@DylanBaker
Copy link
Collaborator

@joshtemple Yeah, I think I am okay with that. Is there value in going back and seeing what version actually introduced the endpoints were are hitting? Are we at all worried that people are using really old versions of Looker?

Ultimately, I think I'm okay removing for the moment, but it would be good to ascertain whether they have actually backported it. I'd also be keen to find a way with Looker to test this on an ongoing basis against a range of instance versions.

@DylanBaker
Copy link
Collaborator

@agile Hey. As @joshtemple suggests, I think the best tact here is to just remove the exception and not have a warning. The fix should be backported so we probably don't need either. Would you mind updating the PR?

@DylanBaker DylanBaker self-requested a review November 13, 2019 23:06
@agile agile force-pushed the less_restrictive_version_check branch 2 times, most recently from c5c148f to ea10cac Compare November 14, 2019 14:21
@DylanBaker
Copy link
Collaborator

@agile I should have also mentioned, we have a Slack community for people who are using and contributing to Spectacles. If you're interested in joining, you can do so here.

@agile agile force-pushed the less_restrictive_version_check branch from ea10cac to 22c2c5b Compare November 14, 2019 14:26
@agile agile force-pushed the less_restrictive_version_check branch from 22c2c5b to 2730da9 Compare November 14, 2019 14:32
@agile
Copy link
Contributor Author

agile commented Nov 14, 2019

Ok, version checking removed ;) Will join slack later

@joshtemple
Copy link
Collaborator

Thanks @agile, we really appreciate the help with this PR!

@joshtemple joshtemple merged commit 394b7f6 into spectacles-ci:master Nov 14, 2019
@agile agile deleted the less_restrictive_version_check branch November 14, 2019 16:05
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.

3 participants