DiamondLightSource / blueapi

Apache License 2.0
3 stars 5 forks source link

Mismatch between Helm and Python versioning schemes #411

Open joeshannon opened 5 months ago

joeshannon commented 5 months ago

When creating, for example, an alpha release using a verion name compatible with the Python Packaging User Guide such as 0.4.1a1 the current CI for publishing the helm chart uses ${GITHUB_REF##*/} as the appVersion. This is rejected by helm/the repository with:


Run sed -i "$ a appVersion: ${GITHUB_REF##*/}" helm/blueapi/Chart.yaml
  sed -i "$ a appVersion: ${GITHUB_REF##*/}" helm/blueapi/Chart.yaml
  helm dependencies update helm/blueapi
  helm package helm/blueapi --version ${GITHUB_REF##*/} -d /tmp/
  helm push /tmp/blueapi-${GITHUB_REF##*/}.tgz oci://ghcr.io/diamondlightsource/charts
  shell: /usr/bin/bash -e {0}
  env:
    GCR_IMAGE: ghcr.io/diamondlightsource/blueapi
    HELM_VERSION: 3.10.3
Error: Invalid Semantic Version
Error: Process completed with exit code 1.

Helm uses Sem Ver 2.0.

There is a library which could help https://python-semver.readthedocs.io/en/latest/advanced/convert-pypi-to-semver.html but other solutions might be preferred or easier.

joeshannon commented 5 months ago

The current state of this is that the PyPI GH action will convert a sem ver tag to a PEP 440 compliant release.

There was some discussion about converting the copier template to enforce a sem ver version however it was decided that this requirement is a special case and should not be the norm as most projects will likely only need/want to be PEP 440 compliant.

Therefore we should be able to use sem ver tags for releases and everything should work but this should probably be documented within this project. Additionally it might be possible to validate this before the release tasks are performed using an action but I haven't explored this yet.

callumforrester commented 5 months ago

I think it's worth documenting higher up, this is a special case but not that special. It's a Copier project with a helm chart, I suspect there will be more than a few of those. Tagging @coretl and @gilesknap for opinions

gilesknap commented 5 months ago

Tom and I discussed this.

We thought about enforcing use of semver in copier but decided that was confusing to the majority pure python projects.

Its a gotcha that is hard to know where to flag.

Perhaps we could add a question in copier. Asking if you will be deploying things other than python packages(such as helm) and add in semvar enforcement in the CI if the answer is yes. (The question could highlight the need for semvar in this case too)

coretl commented 5 months ago

I think if we add helm chart as an option to the copier template then we should do this, and I'd like to see some more usecases for python project with a helm chart in it before we do that. Do we have any apart from blueapi?

callumforrester commented 5 months ago

Presumably any Python backend web service eventually? Adding helm to the copier template might create more awkward than its worth. There are lots of different types of helm template - REST service, epics service etc. so constraining it it seems counterproductive.

coretl commented 5 months ago

I'm not convinced that there will be any epics services, we use ec to build the helm chart for the instance using a helm chart library then adding config. As most of our stuff has config files, I can't see why we would build a helm chart in the repo, just the container. That's the approach for PandA.

I think we should defer this until we have a second example to look at...

callumforrester commented 4 months ago

This is now a blocking concern, has there been any progress on it?

gilesknap commented 4 months ago

@callumforrester I think there is a simple answer which I now realize has not been highlighted in the comments on this issue.

Just use semver for this repo. Helm will be happy and pypi publishing will automatically convert it to PEP440.

I have verified the latter here:

If you want to put in a check into blueapi CI to tell you you have made a bad version then I think that should be a custom change to blueapi itself because we think in general this would be a confusing thing for users of python copier template who just want python packages published.

What do you think?

gilesknap commented 4 months ago

Maybe rather than doing the check which is just another way of failing the CI we could have a troubleshooting section in the docs that says if you are making a beta and this happens then ...

joeshannon commented 4 months ago

We've just made a release of blueapi too to check this works (0.4.1-a3 / PyPI 0.4.1a3) which it did, as expected.

For reference: the version conversion happens when the wheel is built in dist/build action (eventually via some normalisation, see tests in test_version.py).

Maybe rather than doing the check which is just another way of failing the CI

I think we want some check to prevent the case where publishing may succeed to one service but fail on others (as can currently happen).

Then we should add versioning info to this project's docs.

callumforrester commented 4 months ago

@coretl so here we have a working solution for pypi and helm, feels reasonably to move to a copier template issue at this point?

coretl commented 3 months ago

@coretl so here we have a working solution for pypi and helm, feels reasonably to move to a copier template issue at this point?

I'm still waiting for the second example to look at... Has this popped up anywhere apart from blueapi?

callumforrester commented 3 months ago

I presume it would happen if we ever did an alpha release of https://github.com/DiamondLightSource/diffcalc-api

coretl commented 3 months ago

Is there anything that stops you making a GitHub release if it doesn't conform to a regex then?

callumforrester commented 3 months ago

Not sure, that would be ideal. If not, another option is to prevent the release pipeline from triggering unless it conforms to a regex. Then the release just appears in the GH release history and you can yank it when you realise you've got it wrong.