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

fix(setupChecks): update db version checks #45241

Merged
merged 6 commits into from
May 11, 2024
Merged

Conversation

joshtrichards
Copy link
Member

@joshtrichards joshtrichards commented May 9, 2024

Summary

  • Update lowest supported version to match existing docs
  • Adds check for highest supported version that matches existing docs
  • Remove references to Nextcloud Server v21
  • Clarifies the language a bit
  • Adjust CI tests to test the PostgreSQL versions in the docs (drops 10, replaces 15 with 16)

TODO

  • If backported, each one will have to be slightly modified. This is spec'd for v30/master as it stands today.

Follow-up ideas (i.e. out of scope) and other related notes:

Checklist

Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

Adjusting the minimum versions is very good! But I do not think we should add the upper version here.

@joshtrichards
Copy link
Member Author

joshtrichards commented May 9, 2024

But I do not think we should add the upper version here.

Any particular reason?

My thinking:

  • It's in the system requirements docs
  • It's just a warning; it doesn't stop anyone from doing it
  • Some people seem to run unsupported db versions -- both too low and too high -- and it seems it would be useful to let them know that they're environment is out-of-spec... and also for us when difficult to reproduce bugs/odd behaviors are reported

@susnux
Copy link
Contributor

susnux commented May 10, 2024

Makes sense because of the wording ("suggestion").

@susnux susnux force-pushed the checks-db-versions branch from 4ddbe38 to c3efa80 Compare May 10, 2024 16:07
Signed-off-by: Josh Richards <josh.t.richards@gmail.com>
Signed-off-by: Josh Richards <josh.t.richards@gmail.com>
Signed-off-by: Josh Richards <josh.t.richards@gmail.com>
Signed-off-by: Josh Richards <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh Richards <josh.t.richards@gmail.com>
@AndyScherzinger AndyScherzinger merged commit 4560ddd into master May 11, 2024
158 checks passed
@AndyScherzinger AndyScherzinger deleted the checks-db-versions branch May 11, 2024 14:42
Comment on lines +65 to +67
// we only care about X.Y not X.Y.Z differences
[$major, $minor, ] = explode('.', $versionlc);
$versionConcern = $major . '.' . $minor;
Copy link
Contributor

Choose a reason for hiding this comment

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

version_compare is smart enough that you do not need to do this I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

It wasn't when I tried it. It compares the full version string provided. Also I already had to be more specific for the PostgreSQL check anyway (since we only care about major and not even minor for that one).

Copy link
Contributor

Choose a reason for hiding this comment

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

https://3v4l.org/T1lkh It works also if you do not care about minor.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's mostly for the <= check because I wanted the code to literally state the min/max "versions" (release branches) supported just as they're written in the documentation.

Comparing against N+1 feels less self documenting. ;-)

Copy link
Contributor

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants