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

Improve version update detection. #1155

Merged
merged 6 commits into from
Mar 3, 2021

Conversation

softins
Copy link
Member

@softins softins commented Mar 1, 2021

Send version requests on startup to two different central servers.
It doesn't matter if they both reply, so no need to wait and retry.
Only compare if server is running a released version (no suffix).

Send version requests on startup to two different central servers.
It doesn't matter if they both reply, so no need to wait and retry.
Only compare if server is running a released version (no suffix).
@softins softins linked an issue Mar 1, 2021 that may be closed by this pull request
@softins softins requested review from hoffie and pljones March 1, 2021 17:19
@softins softins added this to the Release 3.7.0 milestone Mar 1, 2021
Copy link
Member

@hoffie hoffie left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! Looks got besides spacing nits. :)

Is it worth a Changelog entry?

I tested the following:

  • The new code does not currently trigger an update notification -- as expected.
  • The new code does trigger an update notification when pointing update1.jamulus.io to a server which pretends to run 3.6.3 or 3.7.0 (via /etc/hosts) -- as expected.
  • The new code not does trigger an update notification when pointing update1.jamulus.io to a server which pretends to run 3.6.3beta1(via /etc/hosts) -- as expected.

Only compare if server is running a released version (no suffix).

I think this is a good approach. However, Volker currently runs c1bc427 on central1. This would be no issue for 3.7.0, but we will have to make sure that neither Volker nor @pljones run anything other than final releases on these servers. Would we need to document that somewhere? If so, where?

@softins
Copy link
Member Author

softins commented Mar 1, 2021

Is it worth a Changelog entry?

Probably. I haven't got into the habit of adding those yet, as previously, Volker would add them all when getting a release ready.

I tested the following:

  • The new code does not currently trigger an update notification -- as expected.
  • The new code does trigger an update notification when pointing update1.jamulus.io to a server which pretends to run 3.6.3 or 3.7.0 (via /etc/hosts) -- as expected.
  • The new code not does trigger an update notification when pointing update1.jamulus.io to a server which pretends to run 3.6.3beta1(via /etc/hosts) -- as expected.

Great. My tests were very similar.

Only compare if server is running a released version (no suffix).

I think this is a good approach. However, Volker currently runs c1bc427 on central1. This would be no issue for 3.7.0, but we will have to make sure that neither Volker nor @pljones run anything other than final releases on these servers. Would we need to document that somewhere? If so, where?

Once 3.7.0 is out, the use of suffixes ceases to be a problem, in general (but see next para). The issue we are currently avoiding is putting up a 3.7.0beta server, which 3.6.2 clients (or local servers) would see and show the red message. Older clients don't matter, because there is already a newer version available than they are. Once the current release is 3.7.0, it doesn't matter to older clients if they see 3.7.1beta and put up the message, because it is still true.

With the redundancy, we just need if possible to make sure that one or other of the servers is running a release (at the moment neither is). So yes, Volker and @pljones need to know to check the other one is on a release before trying out a new beta or dev build.

@hoffie
Copy link
Member

hoffie commented Mar 1, 2021

With the redundancy, we just need if possible to make sure that one or other of the servers is running a release (at the moment neither is). So yes, Volker and @pljones need to know to check the other one is on a release before trying out a new beta or dev build.

Yes, that's what I meant.

Updating the current reference server to a beta and using pre 3.7.0 clients would risk a false positive ("update available" message, despite none being there) until 3.7.0 is released.
Updating both new reference servers to a beta and using 3.7.0 clients would risk false negatives (no "update available" message, despite there being one).

@softins
Copy link
Member Author

softins commented Mar 1, 2021

Updating the current reference server to a beta and using pre 3.7.0 clients would risk a false positive (update available, despite none being there) until 3.7.0 is released.

That's right, any betas of 3.7.0 need to be built as 3.6.2dev. We can't build as 3.7.0betaN.

@pljones
Copy link
Collaborator

pljones commented Mar 2, 2021

The issue we are currently avoiding is putting up a 3.7.0beta server, which 3.6.2 clients (or local servers) would see and show the red message.

Which only applies to Volker's server. Anything before 3.7.0 won't be looking at update2.

@ann0see
Copy link
Member

ann0see commented Mar 3, 2021

Ready to be merged?

@softins
Copy link
Member Author

softins commented Mar 3, 2021

Ready to be merged?

Yes, I just want to adjust my last commit, and then I will merge it.

EDIT: In fact, no, I'll commit it as is.

@softins softins merged commit 1985ca5 into jamulussoftware:master Mar 3, 2021
@softins softins deleted the version-check branch March 3, 2021 10:00
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.

Make version check more reliable
4 participants