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

Bring back version check & beacon reporting #7211

Merged
merged 8 commits into from
Nov 6, 2024
Merged

Conversation

arikfr
Copy link
Member

@arikfr arikfr commented Oct 29, 2024

What type of PR is this?

  • Feature

Description

This revert the changes from #6852, which removed the version check. The version check has two important functions:

  1. Allows us to notify admins of when there is a new version. While no new version was released in a while, we're about to package a new one which if it is released without this functionality will prevent us from notifying about future releases.
  2. It serves as a way to get a census of Redash deployments that can help in guiding future decisions (like when we can deprecate some functionality or help page). In the past this data was held private, but I'll look into ways to share it with the community and maintainers.

How is this tested?

  • Manually

@arikfr arikfr requested a review from justinclift October 29, 2024 09:55
@justinclift
Copy link
Member

justinclift commented Oct 29, 2024

One of the things that #6852 addressed was a parsing error for the version string:

worker_1            | [2024-04-04 23:45:51,490][PID:92][ERROR][root] Failed checking for new version (probably bad/non-JSON response).
worker_1            | Traceback (most recent call last):
worker_1            |   File "/app/redash/version_check.py", line 78, in run_version_check
worker_1            |     _compare_and_update(latest_version)
worker_1            |   File "/app/redash/version_check.py", line 97, in _compare_and_update
worker_1            |     is_newer = semver.compare(current_version, latest_version) == -1
worker_1            |   File "/usr/local/lib/python3.8/site-packages/semver.py", line 282, in compare
worker_1            |     v1, v2 = parse(ver1), parse(ver2)
worker_1            |   File "/usr/local/lib/python3.8/site-packages/semver.py", line 65, in parse
worker_1            |     raise ValueError('%s is not valid SemVer string' % version)

If this PR avoids re-introducing that, then I have no super serious objections to it. 😄

@arikfr
Copy link
Member Author

arikfr commented Oct 29, 2024

If this PR avoid re-introducing that, then I have no super serious objections to it. 😄

The reason it failed is because SemVer defines that each segment of the version needs to be an integer without leading zeros. So while 24.04.1 is wrong 24.4.1 is correct.

I can still add some exception handling there, but as long as we follow SemVer rules we should be fine.

@justinclift
Copy link
Member

Ahhh. Sounds like we'd better adjust our version tagging to not include leading zeros then. 😄

@arikfr arikfr mentioned this pull request Oct 29, 2024
2 tasks
@eradman
Copy link
Collaborator

eradman commented Oct 31, 2024

The reason it failed is because SemVer defines that each segment of the version needs to be an integer without leading zeros. So while 24.04.1 is wrong 24.4.1 is correct.

I can still add some exception handling there, but as long as we follow SemVer rules we should be fine.

I don't have a strong opinion on this, but at the time I was actually trying to follow the versioning Ubuntu uses, and they use leading zeros

https://wiki.ubuntu.com/Releases

@arikfr
Copy link
Member Author

arikfr commented Oct 31, 2024

I don't have a strong opinion on this, but at the time I was actually trying to follow the versioning Ubuntu uses, and they use leading zeros

It's not like we religiously follow SemVer so I don't mind supporting leading zeros, but then we need to implement our own version comparison code. Probably not much drama and maybe there is some existing code for this?

@justinclift
Copy link
Member

justinclift commented Oct 31, 2024

Hmmm, we should probably just go with the err... zero-less 😉 semver approach. That way we'll definitely avoid breaking our stuff, and for all we know there might be other things that look at our version string which we're not yet aware of.

So if we drop leading zeros it'll probably have the widest compatibility / least potential for screwing something up? 😄

@arikfr
Copy link
Member Author

arikfr commented Oct 31, 2024 via email

@arikfr
Copy link
Member Author

arikfr commented Nov 5, 2024

Any objections to merging this?

@eradman
Copy link
Collaborator

eradman commented Nov 5, 2024

No objections here

@justinclift justinclift enabled auto-merge (squash) November 6, 2024 00:16
@justinclift justinclift merged commit 349cd5d into master Nov 6, 2024
14 checks passed
@justinclift justinclift deleted the version-check branch November 6, 2024 01:33
@@ -13,7 +13,7 @@ jobs:
steps:
- uses: actions/checkout@v4
with:
ref: ${{ github.event.pull_request.head.sha }}
ref: ${{ github.event.pull_request.head.ref }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did we mean to make this change? Restyled is failing on recent PRs....not clear to me why git checkout is failing.

Copy link
Member

Choose a reason for hiding this comment

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

Seems related to this? #7191 (comment)

@eradman eradman mentioned this pull request Nov 21, 2024
2 tasks
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