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

Validate (at least some) args #251

Merged
merged 17 commits into from
Oct 10, 2023
Merged

Validate (at least some) args #251

merged 17 commits into from
Oct 10, 2023

Conversation

why-not-try-calmer
Copy link
Contributor

Found myself creating a <myplugin_.>.zip file after a typo while running the package command. Thought it might be time to tighten the screws.

@why-not-try-calmer why-not-try-calmer changed the title Validate args Validate (at least some) args Oct 2, 2023
added semver pattern for the most thorough users
compile
for pattern in Parameters.release_version_patterns.values()
):
raise ValueError(
f"Unable to validate the release version '{args.release_version}'. Please use a version name that looks like this: 'v1.1.1', 'v1.1', '1.0.1', '1.1'."
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure we correctly support the prefix v (not checked). Have you tested it?

Copy link
Contributor Author

@why-not-try-calmer why-not-try-calmer Oct 3, 2023

Choose a reason for hiding this comment

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

qgis-plugin-ci is able to package release versions such as v0.1:
image

Whether this is correct depends on who you ask, but if you ask GitHub, I think the answer is 'yes' since you can use such tags both when doing a GitHub Release and when manually tagging commits. If you ask semver fans this answer is 'no', but I thought it would be strict enough to validate git / GitHub-style versions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We have some tests with a v prefix : https://github.com/opengisch/qgis-plugin-ci/blob/master/test/test_changelog.py#L82

So I would say it's OK.

Copy link
Member

Choose a reason for hiding this comment

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

my concern is not the changelog but rather QGIS plugin repo. We just need to be sure the v is dropped from the zip name and possibly in the metadata.

I see 3 approaches:

  1. we need this, so we make sure it works
  2. we don't need it and want to play it safe: we don't advertise it, wait and see (or we don't even support it in the regex)
  3. we don't really care, let this in and wait and see

My choice would be 2, but not strong opinion.

Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI, there are quite a few plugins having a v prefix, either in the ZIP name or in their metadata on the QGIS repository.

Random one https://plugins.qgis.org/plugins/qgis_server_render_geojson/ :)

Here in the QGIS plugin manager : https://github.com/qgis/QGIS/blob/0bef833103724b65b2151afc3bfe63db4c0a6fbf/python/pyplugin_installer/version_compare.py#L60

But indeed, I prefer a proper semver tag as ell ;-)

Copy link
Contributor Author

@why-not-try-calmer why-not-try-calmer Oct 4, 2023

Choose a reason for hiding this comment

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

See changes from commits here below. In hindsight I think it's too strict to raise on faulty release version patterns (NB: this PR doesn't touch the --release_tag arg, which I think doesn't need validation anyway). So I added a flag for people who want to bypass it. There's still a warning when a release version doesn't matches the semver pattern when validation has been disabled. This means that the question of how to handle non-semver-compliant patterns is waived to the user.

So packaging both with and without 'v' is supported provided the version matches a pattern.

@why-not-try-calmer

This comment was marked as resolved.

qgispluginci/parameters.py Outdated Show resolved Hide resolved
@why-not-try-calmer
Copy link
Contributor Author

Thanks for the reviews and suggestions, may I now merge this or do you guys would like more changes?

@3nids 3nids merged commit 582278d into master Oct 10, 2023
2 checks passed
@3nids 3nids deleted the validate-args branch October 10, 2023 06:22
@nyalldawson
Copy link

@3nids @Gustry @why-not-try-calmer

I'm seeing some workflow failures after this change. Specifically, I'm using a workflow like this: https://github.com/north-road/cesium-ion-plugin/blob/main/.github/workflows/build.yml in order to build a testable zip of the plugin from every merge to main. This includes the step:

    - name: Build package
      run: |
        qgis-plugin-ci package dev-${GITHUB_SHA}
        mkdir tmp
        unzip cesium_ion.dev-${GITHUB_SHA}.zip -d tmp

    - uses: actions/upload-artifact@v2
      with:
        name: cesium_ion_plugin
        path: tmp

Prior to this change the workflow was working correctly. Now it fails with:

Traceback (most recent call last):
  File "/opt/hostedtoolcache/Python/3.10.13/x64/bin/qgis-plugin-ci", line 8, in <module>
    sys.exit(cli())
  File "/opt/hostedtoolcache/Python/3.10.13/x64/lib/python3.10/site-packages/qgispluginci/cli.py", line [16](https://github.com/north-road/cesium-ion-plugin/actions/runs/8133626840/job/22225439757#step:5:17)3, in cli
    Parameters.validate_args(args)
  File "/opt/hostedtoolcache/Python/3.10.13/x64/lib/python3.10/site-packages/qgispluginci/parameters.py", line 304, in validate_args
    raise ValueError(
ValueError: 
            Unable to validate the release version 'dev-967f14a89bafc5f037d77afec382d3a801aeb96a'.
            Please use a release version identifier such as '1.0.1' (recommended, semantic versioning), 'v1.1.1', 'v1.1', or '1.1'.
            Otherwise you can disable validation by running this command again with an extra '--no-validation' flag.
            Semantic versioning (semvar) identifiers are recommended.
            Take a look at https://en.wikipedia.org/wiki/Software_versioning#Semantic_versioning for a refresher."

Is this misusing the qgis-plugin-ci tool? Or is there a different setup I should be using in order to provide zips of the master branch for user testing?

@3nids
Copy link
Member

3nids commented Mar 4, 2024

@nyalldawson the version check was introduced here #251

Could you live with adding --no-validation ?

@Gustry @Guts thoughts?

@Gustry
Copy link
Collaborator

Gustry commented Mar 4, 2024

Yes, it's the purpose of this flag it seems (just reading the error message provided :) but indeed, the ValueError is confusing).

@nyalldawson FYI, I would be careful with such "version" in the metadata.txt. The QGIS plugin manager will not be able to parse this version and therefore will not propose any update of the plugin, even after releasing new tags on the master branch. This is IMHO dangerous because "normal" users will keep this "old" ZIP as long as they don't manually reinstall the plugin. No new updates proposal from QGIS.

I didn't check if this behavior is still the same.

That's why I was working on #52. With this PR, we could always have a proper version number in the metadata.txt even with ZIP files generated on commits on a branch. I wanted to be sure that users will have an update from the QGIS plugin manager when making a new tag. I missed some interests on #52 (happy to repoen this PR if needed). I'm using the bash command provided in the PR to compute a version, and also https://opengisch.github.io/qgis-plugin-ci/usage/cli_package.html#additional-metadata to track packages generated on commits (not tags)

@nyalldawson
Copy link

@Gustry @3nids so is no one else using this workflow to build from master branches? I'm curious as to what approaches others are using

@Gustry
Copy link
Collaborator

Gustry commented Mar 4, 2024

I do builds from a master branch.
I use this command to determine the version number.

So I'm sure when a new tag is made on master branch, QGIS plugin manager will propose the update for everyone (people using a ZIP and people using https://plugins.qgis.org).
You can have a look at packages from a master branch here https://packages.3liz.org/pub/lizmap-qgis-plugin/unstable/ with the latest ZIP for instance

There is a ticket #34

@nyalldawson
Copy link

@Gustry have you got any plugins using that in a GitHub workflow?

@Gustry
Copy link
Collaborator

Gustry commented Mar 4, 2024

Done on QuickOSM just now to try https://github.com/3liz/QuickOSM/blob/master/.github/workflows/ci.yml#L96

      - uses: actions/checkout@v4
        with:
          # To fetch tags
          fetch-depth: 0

      - name: Set up Python 3.10
        uses: actions/setup-python@v5
        with:
          python-version: '3.10'
          cache: "pip"
          cache-dependency-path: "requirements/packaging.txt"

#      - name: Install Qt lrelease for translations
#        run: |
#          sudo apt-get update
#          sudo apt-get install qttools5-dev-tools

      - name: Install Python requirements
        run: pip install -r requirements/packaging.txt

      - name: Set env
        run: |
          TAG=$(git describe --tags $(git rev-list --tags --max-count=1))
          echo "VERSION=$(echo ${TAG} | awk -F. -v OFS=. 'NF==1{print ++$NF}; NF>1{if(length($NF+1)>length($NF))$(NF-1)++; $NF=sprintf("%0*d", length($NF), ($NF+1)%(10^length($NF))); print}')-alpha" >> $GITHUB_ENV

      - name: Package
        run: >-
          qgis-plugin-ci
          package ${{ env.VERSION }}

      - name: Unzip
        run: unzip QuickOSM.${{ env.VERSION }}.zip -d tmp

      - uses: actions/upload-artifact@v2
        with:
          name: QuickOSM.${{ env.VERSION }}
          path: tmp

I just have qgis-plugin-ci in the packaging.txt file.

Latest tag on the repo is 2.2.3. The artifact is called QuickOSM.2.2.4-alpha , so when a tag, obviously higher than 2.2.4-alpha, QGIS plugin manager will propose the update.

@3nids
Copy link
Member

3nids commented Mar 4, 2024

Main downside of this is the need to fetch the complete history.
Another approach would be to use gh instead to get the latest releage/tag (but that's Github specific)

@Gustry
Copy link
Collaborator

Gustry commented Mar 4, 2024

Indeed. GH checkout actions, there is a fetch-tags, maybe it works.
https://github.com/actions/checkout

Maybe I should revive #52, it can switch between either git or the CHANGELOG.md file to know what was the latest tag.

@nyalldawson
Copy link

@Gustry thank you! That works for me, although I also had to still specify the no-validation argument

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.

4 participants