-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Conversation
One of the things that #6852 addressed was a parsing error for the version string:
If this PR avoids 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 I can still add some exception handling there, but as long as we follow SemVer rules we should be fine. |
Ahhh. Sounds like we'd better adjust our version tagging to not include leading zeros then. 😄 |
Co-authored-by: Restyled.io <[email protected]>
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? |
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? 😄 |
On second thought Justin is right: old deployments of Redash will keep
using the semver library for version comparison.
|
Any objections to merging this? |
No objections here |
@@ -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 }} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
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:
How is this tested?