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 min_qg_version query in plugins.xml #224

Merged
merged 3 commits into from
Apr 12, 2022

Conversation

sumandari
Copy link
Collaborator

This is to fix #223

Proposed solution:
Add the 3rd segment with max number as a patch number version (eg. 3.6.99) in qgis version against the query, so that the PluginVersion with minimum qgis version 3.16.x will be available in QGIS 3.16

image

@codecov-commenter
Copy link

codecov-commenter commented Dec 2, 2021

Codecov Report

Merging #224 (4cd2894) into master (4f9990d) will increase coverage by 0.27%.
The diff coverage is 25.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #224      +/-   ##
==========================================
+ Coverage   37.27%   37.55%   +0.27%     
==========================================
  Files          77       85       +8     
  Lines        3364     3672     +308     
==========================================
+ Hits         1254     1379     +125     
- Misses       2110     2293     +183     
Impacted Files Coverage Δ
qgis-app/plugins/views.py 18.50% <5.88%> (-0.12%) ⬇️
qgis-app/plugins/tests/test_view.py 44.44% <44.44%> (ø)
qgis-app/base/forms/processing_forms.py 66.66% <0.00%> (-11.91%) ⬇️
qgis-app/settings_docker.py 100.00% <0.00%> (ø)
qgis-app/layerdefinitions/file_handler.py 21.05% <0.00%> (ø)
qgis-app/layerdefinitions/admin.py 100.00% <0.00%> (ø)
...is-app/layerdefinitions/tests/test_file_handler.py 42.85% <0.00%> (ø)
qgis-app/layerdefinitions/models.py 86.36% <0.00%> (ø)
qgis-app/layerdefinitions/tests/test_forms.py 66.66% <0.00%> (ø)
...is-app/layerdefinitions/migrations/0001_initial.py 100.00% <0.00%> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f9990d...4cd2894. Read the comment docs.

@dimasciput
Copy link
Collaborator

LGTM, but wait for Tim's approval too.

@timlinux will there be any problems on the qgis side?

@rduivenvoorde
Copy link
Contributor

@sumandari looking at your fix: will it be OK to always add .99 ?

Should it not depend on the use of 'minimum_version' and 'maximum_version'?

In the case of somebody telling: minimum version = 2.2 they probably mean: that you should at least have 2.2.0 (and not 2.2.99)?

But maybe this does not affect plugins.qgis.org... ?
What is exactly the comparison we are making?

Another observation: am I right that we are not using some kind of module/code for this, but we are parsing version numbers ourselves? See #236 (comment)

@sumandari
Copy link
Collaborator Author

sumandari commented Jan 27, 2022

Thank you for your feedback @rduivenvoorde. What I proposed in my quick fix was with an assumption that the version of QGIS will never have 3rd versioning segment (patch) in this repo.
It will assume QGIS version 3.16 as 3.16.99 since in this repo we haven't generated xml plugin with 3rd versioning segment

'1.8', '2.0', '2.2', '2.4', '2.6', '2.8', '2.10',

But indeed that's not a good approach for permanent fix. I think #236 (comment) is better and looks promising.

@rduivenvoorde
Copy link
Contributor

@sumandari I updated the #236 (comment) example.

I think this could indeed be very handy and helpfull. It is apparently used for the same purpose in pip to determine the compatibility of modules from the requirements.txt. See more info in the comment.

Thanks for fixing this!!

@sumandari
Copy link
Collaborator Author

Thank you for the update @rduivenvoorde. I'm still figuring out how to change the char type record (min_qg_version) as a version instance so that it can be filtered directly in django query

filters.update({'pluginversion__min_qg_version__lte' : qg_version})

@dimasciput
Copy link
Collaborator

Hi @rduivenvoorde, from your example so we can expect this case :

Because currently the api only accepts two parts numbers : XX.XX

@rduivenvoorde
Copy link
Contributor

rduivenvoorde commented Jan 28, 2022

Hi @dimasciput, not sure if your comment is a question or not.

But I think your scenario is indeed a problem (at this moment?). As apparently the plugin author want's to be able to set a very precise 'patch'-version to it's plugin... And that will indeed not work because QGIS only requests plugins with a major.minimal version number

So isn't it the only solution:

  • either ALSO take a patch version into account and maybe 1) force the use of patch versions in the metadata.txt AND 2) in the plugins.xml url (though this is pretty hard to accomplish backward compatible I think, as we then keep the problem how to handle te major.minor cases, though that could maybe be handled by using the version-module?)
  • OR never use the patch-version anywhere, BUT then we need logic in which 1) the minimal version is rounded DOWN (so 3.16.1 to >= 3.16, and the maximal version is rounded UP (so 3.16.1 to < 3.17 (or <- 3.16.999)?
    And note that Setting 3.22 as maximum version in plugin metadata.txt is used as 3.22.0 (while we think it should be 3.22.99) #236 is about exactly this issue: setting a max_version to 3.22 (which was rounded to 3.22.0)

@sumandari so I think if we take the second solution (which is good enough), then this PR is almost the final solution? Except that in this part:

https://github.com/qgis/QGIS-Django/pull/224/files#diff-9aea4b481c8535529c4c543973b08cfd08e42bb2f8b2ee19a5b268f15abca7e2R1066-R1072

you should also 'patch/fix' the major.minor of max_qg_version?

@suricactus what do you think?

@sumandari
Copy link
Collaborator Author

@rduivenvoorde

@sumandari so I think if we take the second solution (which is good enough), then this PR is almost the final solution? Except that in this part:
https://github.com/qgis/QGIS-Django/pull/224/files#diff-9aea4b481c8535529c4c543973b08cfd08e42bb2f8b2ee19a5b268f15abca7e2R1066-R1072

If we add .99 for filtering max version, it will not include the major.minor with lower patch version
qgis version = 3.22
max version = 3.22.2
max_qg_version >= qg_version
3.22.2 >= 3.22.99 will return false, and the plugin won't be in query result

@rduivenvoorde
Copy link
Contributor

@sumandari I'm not following exactly... but what I thought:

  • qgis version will never have patch-part (as we only use major.minor)
  • plugins sometimes use major.minor.patch, and sometimes major.minor
  • so what I thought to propose (unaware of code implications):
  • for testing against plugin_minimal_version param: test with a qgis-version with patchnr .0 so: plugin >= m.m.0
  • for testing against plugin_maximal_version param: test with a qgis_version with patchnr .99 so: plugin <= m.m.99
    Or am I mixing up stuff?

@suricactus
Copy link

suricactus commented Jan 31, 2022

@rduivenvoorde

I do agree with your proposal, except the part saying "qgis version will never have a patch-part".

I believe QGIS should start sending the patch version too. This should not break anything, or at least nothing worse than now.

The plugin can have either major.minor in their metadata (x.y), or major.minor.patch (x.y.z). This goes for both min and max QGIS version. On the other hand, for backwards compatibility, QGIS can send either major.minor (a.b) or the future versions should send major.minor.patch (a.b.c)

Minimum version check:

plugin meta\URL param from qgis a.b a.b.c
x.y x.y.99 >= a.b.0 x.y.99 >= a.b.c
x.y.z x.y.z >= a.b.0 x.y.z >= a.b.c

Maximum version check:

plugin meta\URL param from qgis a.b a.b.c
x.y x.y.0 <= a.b.99 x.y.0 <= a.b.c
x.y.z x.y.z <= a.b.99 x.y.z <= a.b.c

It is predictable and works as most package manager already work.

For example a plugin with minVersion=3.22 and maxVersion=3.24.4 should be returned to a client sending qgisVersion=3.22.5, qgisVersion=3.23, qgisVersion=3.24, but not for qgisVersion=3.24.5.

@sumandari
Copy link
Collaborator Author

Updated PR 4cd2894:

Test against max_qg_version
e.g plugin: SpatiaLite Manager

image

image

For test against min_qg_version please see: #224 (comment)

@timlinux
Copy link
Member

LGTM @sumandari but:

  1. Can we deploy on a staging server somewhere and have a couple of wells for people to tease I'd before deploying into prod

  2. Please make a persistent machine snapshot before upgrading, so we can roll back easily if we want to

@sumandari
Copy link
Collaborator Author

Can we deploy on a staging server somewhere and have a couple of wells for people to tease I'd before deploying into prod

@timlinux at the moment, we don't have a staging server for QGIS-Django. Could you please advise where we can deploy and test it?

@pathmapper
Copy link
Contributor

Hi, just discovered this PR.

I believe QGIS should start sending the patch version too.

I think so, too (see #222 (comment) for an issue related to this) and opened qgis/QGIS#47907 for the QGIS part to send the patch version.

There are quite some plugins already which are using the patch version for min/max QGIS version.

If you agree, we would need also the api to accept the full version (three part number)

Because currently the api only accepts two parts numbers : XX.XX

e.g. https://plugins.qgis.org/plugins/plugins.xml?qgis=3.16.14

@pathmapper
Copy link
Contributor

So what blocking us moving forward is the question whether a staging server is available?

@timlinux
Copy link
Member

timlinux commented Apr 8, 2022

@pathmapper we are busy setting up a staging server.

@dimasciput dimasciput changed the base branch from master to develop April 12, 2022 08:19
@dimasciput
Copy link
Collaborator

Merging this PR to develop branch and then will deploy it to staging.

@dimasciput dimasciput merged commit 88b3ede into qgis:develop Apr 12, 2022
@dimasciput
Copy link
Collaborator

Hi @pathmapper , we have set up the staging server here https://staging.plugins.qgis.org/. This PR is deployed to the staging server.

@pathmapper
Copy link
Contributor

@pathmapper
Copy link
Contributor

What is the current status here? Are there any issues which are blocking this for production deployment?

dimasciput pushed a commit to dimasciput/QGIS-Django that referenced this pull request Aug 21, 2022
* fix min_qg_version query

* added 0 patch value for qgis version against max_qg_version

* only add patch if it has major.minor version
dimasciput added a commit that referenced this pull request Aug 21, 2022
* Fix min_qg_version query in plugins.xml (#224)

* fix min_qg_version query

* added 0 patch value for qgis version against max_qg_version

* only add patch if it has major.minor version

* Update docker-compose and dockerfile

* Update nginx configuration

* Update smtp

* Add celery beat

* Add feedjack update celery task

* Add metabase configuration

* Update test.yaml

* Add test docker-compose

Co-authored-by: sumandari <sumandari@kartoza.com>
@pathmapper
Copy link
Contributor

pathmapper commented Oct 12, 2022

@dimasciput @timlinux currently for this patch the staging server isn't available anymore and it hasn't reached the production server. Do you have any information about what is planned regarding implementing this for production?

Asking because the corresponding PR in the QGIS repo qgis/QGIS#47907 is marked as stale regularly since months and I'm unsure wether it should be kept open or closed...

@dimasciput
Copy link
Collaborator

Hi @pathmapper , it seems this only merged to develop branch, I will create a PR against master and then deploy it. I will let you know when it's on production.

dimasciput pushed a commit to dimasciput/QGIS-Django that referenced this pull request Oct 12, 2022
* fix min_qg_version query

* added 0 patch value for qgis version against max_qg_version

* only add patch if it has major.minor version
dimasciput pushed a commit to dimasciput/QGIS-Django that referenced this pull request Oct 12, 2022
* fix min_qg_version query

* added 0 patch value for qgis version against max_qg_version

* only add patch if it has major.minor version
dimasciput added a commit that referenced this pull request Oct 12, 2022
* fix min_qg_version query

* added 0 patch value for qgis version against max_qg_version

* only add patch if it has major.minor version

Co-authored-by: sumandari <sumandari@kartoza.com>
@dimasciput
Copy link
Collaborator

sorry @pathmapper for late reply, but this has been deployed to production.

@pathmapper
Copy link
Contributor

@dimasciput no worries, thanks for the info!

https://plugins.qgis.org/plugins/plugins.xml?qgis=3.26.3 gives

grafik

But for staging it's working again
https://staging.plugins.qgis.org/plugins/plugins.xml?qgis=3.26.3

Could you please have a look, why it's not working for the production server?

@dimasciput
Copy link
Collaborator

@pathmapper yes this wont work because the production pre-generated the plugins xml, and the staging doesn't do that.
If we don't generate the plugins file, the server will crash since a lot of people requesting plugins every seconds.
And I don't think it's a good idea to generate each patch number at this moment.

Please check previous discussions #224 (comment)

@pathmapper
Copy link
Contributor

Thanks for the backgound @dimasciput.

the production pre-generated the plugins xml

Does this happen with every plugin upload?

And I don't think it's a good idea to generate each patch number at this moment.

OK, I see.

If we want to proceed with qgis/QGIS#47907 :

Once this PR is merged, QGIS will start to send the patch number so this could happen at the earliest with 3.28.1.
So we would need a plugins xml returned starting with this version. Probably the plugins xml must be avaialble also for develepment versions e.g. currently 3.29.0.

Could this be addressed somehow?

@agiudiceandrea
Copy link
Contributor

agiudiceandrea commented Nov 11, 2022

Hi @sumandari, @dimasciput, @pathmapper, was the issue reported at https://lists.osgeo.org/pipermail/qgis-developer/2022-November/065249.html triggered by this PR?

It appears that the plugins repo is currently stuck serving up outdated
plugins information. Newly published plugin versions aren't showing, and
newly published plugins aren't available.

@dimasciput
Copy link
Collaborator

@agiudiceandrea hi, let me check it now.

@dimasciput
Copy link
Collaborator

@agiudiceandrea yes you are right, I have reverted this PR and will be doing more testing on staging again. Sorry for the incident.

@dimasciput
Copy link
Collaborator

the plugins should be updated now

@agiudiceandrea
Copy link
Contributor

@dimasciput thank you very much!

dimasciput added a commit that referenced this pull request May 6, 2024
* Upgrade the stack from Django 2.2 to 3.2 (#233)

* Upgrade the stack from Django 2.2 to 3.2

* Upgrade pip as well when installing

Co-authored-by: Dimas Ciputra <dimas.ciputra@gmail.com>

* Update docker compose (#256)

* Fix min_qg_version query in plugins.xml (#224)

* fix min_qg_version query

* added 0 patch value for qgis version against max_qg_version

* only add patch if it has major.minor version

* Update docker-compose and dockerfile

* Update nginx configuration

* Update smtp

* Add celery beat

* Add feedjack update celery task

* Add metabase configuration

* Update test.yaml

* Add test docker-compose

Co-authored-by: sumandari <sumandari@kartoza.com>

* Fix docker-compose and dockerfile

* Init starting docker dev environment

* Fixing docker container conflict, updating whoosh

* Update testfiles and feedback test

* Update dockerfile for dev and prod

* Use main branch for whoosh in requirements

* Django 4 update: Requirements, dockerfile and docker-compose

* Django 4 update: fixes for ifequal, ugettext_lazy, django.conf.urls.url occurences

* Django 4 update: Fix deprecated readfp

* Django 4 update: update dbrestore in Makefile

* Django 4 update: Get static, media and backup volumes from the environment variable

* Django 4 update: Use solr thumbnail default engine

* Django 4 update: Fix Django warnings

* Django 4 update: fix depecated tests, new migrations

* Django 4 update: Update django unit tests

* Django 4 update: Add email environment variables

* Django 4 update: Nginx and uwsgi updates

* Django 4 update: Refactoring dockerfile

* Update makefile to use new docker compose

* Django 4 update: Specify devweb container_name, fix typo

* Use existing nginx configuration

* Add DEFAULT_PLUGINS_SITE to environment variables

* Add nginx dev and prod configuration files

* Django 4 update: Generate a .env file in GH actions

* Django 4 update: Fix typo in Makefile

* Django 4 update: Use updated docker compose in test.yaml

* Django 4 update: Certbot service and SSL configuration

* Add a http configuration for Nginx

* Django 4 update: add SSL cert renewal script

* Redirect http to https

---------

Co-authored-by: Étienne Trimaille <gustrimaille@yahoo.fr>
Co-authored-by: Dimas Ciputra <dimas.ciputra@gmail.com>
Co-authored-by: sumandari <sumandari@kartoza.com>
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.

Plugin list does not contain plugins with patch versions
8 participants