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

Support altinity cloud when fetching server version #7

Merged
merged 1 commit into from
Jan 27, 2022

Conversation

macobo
Copy link

@macobo macobo commented Jan 27, 2022

SELECT version() currently returns 21.8.13.1.altinitystable on our demo environment which blows up.

This PR makes it so we strip out the non-numeric suffic on the version.

Related charts PR: PostHog/charts-clickhouse#276

SELECT version() currently returns `21.8.13.1.altinitystable` on our
demo environment which blows up.

This PR makes it so we strip out the version.
@macobo macobo requested a review from guidoiaquinti January 27, 2022 12:43
@@ -410,6 +410,8 @@ def _get_server_version(self, as_tuple=True):
except ServerError as e:
logger.exception('Cannot determine server version (%s), assuming 1.1.0', e)
ver = '1.1.0'
# :TRICKY: Altinity cloud uses a non-numeric suffix for the version, which this removes.

Choose a reason for hiding this comment

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

LGTM

Non-blocking: I'm new to this repo but can we add a test case for this? Also, should we use something like https://packaging.pypa.io/en/latest/version.html instead?

Copy link
Author

Choose a reason for hiding this comment

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

I'd love to add a test, but sadly adding that support seems like would require a lot of work right now. Going to punt on this.

@macobo macobo merged commit 6e2e2b7 into develop Jan 27, 2022
macobo added a commit to PostHog/posthog that referenced this pull request Jan 27, 2022
macobo added a commit to PostHog/posthog that referenced this pull request Jan 27, 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