-
Notifications
You must be signed in to change notification settings - Fork 279
Autocheck for new DOMjudge releases #3055
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
Conversation
18e58bb
to
378db01
Compare
f2a225c
to
de80c58
Compare
preg_match("/\d.\d.\d/", $this->domjudgeVersion, $matches); | ||
$extractedLocalVersionString = $matches[0]; | ||
if ($this->config->get('check_new_version', false) === UpdateStrategy::Strategy_incremental) { | ||
/* Steer towards the nearest new patch release first |
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.
I might be alone with this opinion, but I think we should tell users the most recent version instead.
Feel free to close
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.
I've now added both strategies.
I can see a case for both strategies if we do enough of the work (and had the code already for both),
We should discuss this in person though as to me doing the patch releases is that extra time which I don't think we invest often enough. We change quite a lot between versions and we seem to forget to backport often so the patch releases are often very outdated to maintain and not often useful because the fixes are not being backported (and therefor a sort of false promise).
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.
I think that having both strategies is nice, if we actually do the bugfix backports. If we do not plan to more actively do these, then I agree that we should guide users towards the latest version.
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.
I think we should do backports/releases more indeed and I think offering both patch and major/minor release updates makes sense. Why can't we display both at the same time?
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.
Have we people asking for backport releases? They are a lot of work and I'm not sure whether it's worth the effort
webapp/src/Twig/TwigExtension.php
Outdated
'doc_links' => $this->dj->getDocLinks(), | ||
'allow_registration' => $selfRegistrationCategoriesCount !== 0, | ||
'enable_ranking' => $this->config->get('enable_ranking'), | ||
'new_version_available' => $this->dj->checkNewVersion(), |
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.
From what I see and tried, this will make a HTTP request to versions.domjudge.org
on every page load. This seems very aggressive, and might even cause an accidental DDOS of our server when large contests are running. Should we store it with some cache time in the DB?
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.
Given where the server is running I doubt if that ever happens but we can handle that case. I don't prefer to store this in the database but we can use this:
https://symfony.com/doc/current/components/cache.html
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.
Alternatively just do it on the index page?
/** | ||
* @return String[]|false | ||
*/ | ||
public function cacher_checkNewVersion(ItemInterface $item): array|false { |
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.
I'll rename this one.
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.
Caching seems to work as expected, thanks!
Store optional installation method for install metrics, we can use this to see what to focus efforts on.
Discussed during the hackathon in 2023. We allow admins to toggle an autoquery to domjudge.org for 2 reasons: - Alerting users that a new version might exist, helping us in case of security releases. - Giving a gentle nudge for people to upgrade making support easier. - Getting some information on what installations are out there. Co-authored-by: Tobias Werth <[email protected]>
I think this was merged prematurely, as https://versions.domjudge.org doesn't exist yet. |
When the site is not reachable it should just do nothing. So if you get problems in your local copy something is wrong. |
Discussed during the NWERC hackathon in 2023.
We allow admins to toggle an autoquery to domjudge.org for 2 reasons:
Default this doesn't happen, so the admin chooses to enable this themselves so this is different from normal telemetry.