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

Mismatch between Helm and Python versioning schemes #411

Open
joeshannon opened this issue Apr 3, 2024 · 23 comments
Open

Mismatch between Helm and Python versioning schemes #411

joeshannon opened this issue Apr 3, 2024 · 23 comments
Labels
bug Something isn't working github_actions Pull requests that update GitHub Actions code
Milestone

Comments

@joeshannon
Copy link
Contributor

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 joeshannon added bug Something isn't working github_actions Pull requests that update GitHub Actions code labels Apr 3, 2024
@joeshannon
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor

gilesknap commented Apr 11, 2024

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
Copy link
Contributor

coretl commented Apr 11, 2024

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
Copy link
Collaborator

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
Copy link
Contributor

coretl commented Apr 11, 2024

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
Copy link
Collaborator

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

@gilesknap
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Collaborator

@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
Copy link
Contributor

coretl commented May 15, 2024

@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
Copy link
Collaborator

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

@coretl
Copy link
Contributor

coretl commented May 15, 2024

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

@callumforrester
Copy link
Collaborator

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.

@stan-dot stan-dot added this to the 1.0.0 milestone Jun 4, 2024
@stan-dot
Copy link
Collaborator

have we had any issues with this around the 0.5.0 release? I think not, @callumforrester , maybe this can be closed

@callumforrester
Copy link
Collaborator

@stan-dot There are no issues because we know how to avoid them (tag releases with 1.2.3-a1 rather than 1.2.3a1). This issue is still open because we would like more foolproofing than that. For example, a nice error message when you use the wrong format, or a job that automatically converts between the two formats.

@stan-dot
Copy link
Collaborator

related issue
helm/helm#9728

@stan-dot
Copy link
Collaborator

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).

@joeshannon I'm having issues trying to replicate this

@callumforrester
Copy link
Collaborator

I think we can solve this with tag rulesets: https://docs.github.com/en/[email protected]/repositories/managing-your-repositorys-settings-and-features/managing-repository-settings/configuring-tag-protection-rules

I will experiment with it at some point, documenting it here in case anyone else has time sooner than me.

@stan-dot
Copy link
Collaborator

0.4.1-a3 as a valid tag name and 0.4.1a3 as invalid - and to put the regexes to confirm this and reject other tags? testing this out...

@stan-dot
Copy link
Collaborator

this should work, but I don't have the permissions

^\d+\.\d+\.\d+-[a-zA-Z]+\d+$

to match the dash and this, I think:

Require deployments to succeed

@callumforrester
Copy link
Collaborator

I had a play with that, update on how far I got:

  1. Rather than spam this repo with testing tags I made one in my own Github namespace to play with: https://github.com/callumforrester/ruleset-playground
  2. Your regex is slightly incomplete, it should actually be ^\d+\.\d+\.\d+(-[a-zA-Z]+\d+)?$ to allow for normal releases as well (e.g. 1.2.3)
  3. As it turns out Github uses fnmatch rather than regex to define rulesets, I haven't yet figured out how to define semver in fnmatch or if it's even possible
  4. There is an option for regex but it is a Github Enterprise feature

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working github_actions Pull requests that update GitHub Actions code
Projects
None yet
Development

No branches or pull requests

5 participants