getredash / redash

Make Your Company Data Driven. Connect to any data source, easily visualize, dashboard and share your data.
http://redash.io/
BSD 2-Clause "Simplified" License
26.45k stars 4.38k forks source link

Bring back version check & beacon reporting #7211

Closed arikfr closed 2 weeks ago

arikfr commented 3 weeks ago

What type of PR is this?

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?

justinclift commented 3 weeks ago

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. :smile:

arikfr commented 3 weeks ago

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 commented 3 weeks ago

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

eradman commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

Hmmm, we should probably just go with the err... zero-less :wink: 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? :smile:

arikfr commented 3 weeks ago

On second thought Justin is right: old deployments of Redash will keep using the semver library for version comparison.

arikfr commented 2 weeks ago

Any objections to merging this?

eradman commented 2 weeks ago

No objections here