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

Add major-minor-release workflow #45

Draft
wants to merge 8 commits into
base: unstable/v1
Choose a base branch
from

Conversation

s-weigand
Copy link

Here is the promised workflow, for major and minor tag releases, which works with PEP440 versions.

Creates major+minor tags on

  • 'normal' tags (e.g. v2.0.0)
  • post tags (e.g. v2.0.0.post0)

Does not creates major+minor tags on

  • dev tags (e.g. v2.0.0.dev0)
  • pre tags (e.g. v2.0.0.rc0)

Also, it saves the original tag name as an annotation in the major and minor tags.

I tested some example tags on my fork.

@@ -0,0 +1,33 @@
name: "Publish Major-Minor-Tags"
on:
push:
Copy link
Member

Choose a reason for hiding this comment

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

Honestly, I'm lately preferring the create event because it has closer semantics to what we're attempting to trigger. But using it requires a post-condition.

WDYT?

Copy link
Author

Choose a reason for hiding this comment

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

I mostly use push due to sheer habit, there were a lot fewer triggers (at least in the docs) when I started adopting gh-actions as soon as they came out of beta.
The docs are kinda unclear on how to use it, would simply changing push for create work? Or what is the proper pattern, only to trigger on tags that start with v

I guess semantically the closes would be to use release.

on:
  release:
    types: [published]

I use this trigger for my actions and it works fine, but I'm not sure how this will play out with dev, pre and post tags, since those will most likely not published.
The other release type to use would be created, which wouldn't force one to release on the markedplace, but it could still happen by accident (I think to remember that the checkbox is checked by default after fist publishing).

Copy link
Member

Choose a reason for hiding this comment

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

The usage of create is
It's

on:
  create:

jobs:
  job-name:
    if: github.event.ref_type == 'tag'  # to filter out branch creation

(because it doesn't have "native" filters)

I usually create the tags using local Git and push them from CLI. And only after that I publish releases. So I'm not sure if release would be suitable. OTOH this workflow could auto-publish a release on tag creation. You already have a way of saying if the "pre-release" checkbox should be set.

Also, I'm fine with publishing pre-releases to Marketplace. I've done it before.
See https://github.com/pypa/gh-action-pypi-publish/releases/tag/v1.0.0a0 — it has a pre-release checkbox. And it's marketplace page https://github.com/marketplace/actions/pypi-publish?version=v1.0.0a0 also has a pre-release marker, just like PyPI.

Copy link
Author

Choose a reason for hiding this comment

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

I think auto-publishing is a bit too much automation since the release messages often need some handcrafted work. e.g. markdown formatting, which can't be extracted from the tag.

As for the published event, I don't know how github determines what the current release is, my guess is that it sorts them by published date. If that assumption is correct a post release could mess up what version is displayed on the marketplace. E.g. you have a v2.0.0 release and after that release v1.4.1.post0 to trigger the workflow, v1.4.1.post0 might be shown a current version. So IMHO [created, edited] would be closer to the desired behavior.
Also using release would make it more reasonable to add a link to the release to the created tags (see #45 (comment)).

As for the semantics of create vs. push, I think this boils down to different reference systems remote vs. locally (sorry physics student here 😆 ).
I personally prefer the more concise syntax of push, where

on:
  push:
    tags:
      - "v*"

gives you the same functionality as

on:
  create:

jobs:
  job-name:
    if: github.event.ref_type == 'tag' && startsWith( github.ref, 'v')

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about the markdown part. First of all, it's possible to have tag annotations with markdown baked-in but I wouldn't care about it too much: when you mark a release as published to the Marketplace, the new version appears there but the release description does not show up there. Instead, it just contains README content.
So it's okay to just auto-publish and then, if needed, I'd just correct it.
FWIW we shouldn't care about ordering the tags: it'll be like this regardless of whether a robot or a human makes the release... And we're not going to avoid putting .post tags on the marketplace while moving the major tag there in Git.

@s-weigand s-weigand marked this pull request as draft November 3, 2020 23:13
git config user.email 'github-actions[bot]@users.noreply.github.com'
git config user.name 'github-actions[bot]'
git tag --annotate '${{ env.major_version }}' \
--message='original tag: ${{ env.original_tag_name }}'
Copy link
Member

Choose a reason for hiding this comment

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

have you considered appending the message from the original tag too?

Suggested change
--message='original tag: ${{ env.original_tag_name }}'
--message='original tag: ${{ env.original_tag_name }}' \
--message="$(git tag -l --format='%(contents)' '${{ env.original_tag_name }}')"

Copy link
Author

Choose a reason for hiding this comment

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

This would only work as intended if the tag had an additional message to it or?
E.g. v1.4.0 works fine

$ git tag -l --format='%(contents)' 'v1.4.0'
Merge PR #39

This change exposes an extra GitHub Action input called `verbose`. It
allows to see `twine upload` detailed output if needed for debugging.

while v1.4.1 doesn't

$ git tag -l --format='%(contents)' 'v1.4.1'
v1.4.1
-----BEGIN PGP SIGNATURE-----
.
.
.
-----END PGP SIGNATURE-----

IMHO adding a link to the release would be more informative.

Copy link
Member

Choose a reason for hiding this comment

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

In the second case, v1.4.1 is also an additional message because all signed tags are also annotated. While I understand the sentiment of having a link, I'm not sure if it could be achieved easily. Another thing that would be useful is the commit hash where the tag points at (security-wise).

Copy link
Author

Choose a reason for hiding this comment

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

With some conditions, creating a link would be pretty straight forward:

  • release name always needs to be the same as tag name
  • workflow triggers on
    on:
      release:
       types: [created, edited]

If those conditions are met, the link would be
https://github.com/pypa/gh-action-pypi-publish/releases/tag/${{ env.original_tag_name }}

As for adding the SHA1 of the commit, this only provides additional information in a CLI context, since the github website already points to the underlying commit (see test tag v2.0.0).

Copy link
Member

Choose a reason for hiding this comment

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

As for adding the SHA1 of the commit, this only provides additional information in a CLI context, since the github website already points to the underlying commit (see test tag v2.0.0).

Besides being a pointer, it's also proof that it was created by the workflow and not manually, overriding it to some malicious commit.

Co-authored-by: Sviatoslav Sydorenko <[email protected]>
@s-weigand s-weigand force-pushed the add-major-minor-releases branch 3 times, most recently from 534a0e6 to 22ec670 Compare November 9, 2020 16:33
@webknjaz
Copy link
Member

@s-weigand it has recently been brought to my attention that rewriting tags is a terrible idea. They are supposed to be immutable. It's branches that actually need to be used as a floating pointer. It seems like the best way of implementing this is to actually use branches with names like v1 / v2 and just make sure that the corresponding tags actually belong there. WDYT?

@s-weigand
Copy link
Author

@webknjaz While it is true that the idea behind tags is that they are immutable, github themselves rewrites them for major releases of their own actions (e.g. https://github.com/actions/cache/releases).
I can only speak for myself but I really seldom look at tags of a repo, but quite often at the branch tree (especially with GUI tools) and having many branches would clutter that.
So my opinion on this is so to say 'When in Rome, do as the Romans do.' and I would stick with the pattern github uses for its own actions.

Also, the tag of the exact release stays immutable, so the way I see minor or major tags, in general, isn't as strict tag but more like a convenience thing equivalent to something pip install jupyter~=2.1. If you want the assurance that you get the exact version you want, for one reason or the other, you can always specify the fully qualified version.

@webknjaz
Copy link
Member

webknjaz commented Dec 6, 2020

Well, I'd not idealize what GH does — they often make questionable decisions. I think it's better to have branches called v1/v1.1/v1.2, and so on. But for the patch releases, there would be actual tags. When the minor version is bumped, the previous branch could be turned into a tag and removed so the number of branches wouldn't be too high.
I'm not sure if the development should be kept in the main branch or if the major like v1 should become a repo default, though.

And yes, I agree that patch versions should be used mostly — dependabot even knows how to bump those. But what I'm saying is that all the tags must stay immutable over time, no exceptions. Just because somebody else breaks the rules doesn't mean that it's a good idea to subvert Git's ideology. They still haven't solved the security issue that arises from using moving targets on the platform level meaning that all the users have to either use trusted Actions or use commit SHAs and audit every change on their own since there's no fair guarantee that some random action author won't go rogue and retargets a tag or a branch to some malicious commit (it's even more dangerous when done silently and temporarily — it leaves almost no trace in logs).

@s-weigand
Copy link
Author

Ok point taken.
Also, I wasn't idealizing the decision of GH to overwrite tags. It was more like that the meaning of minor/major tags are different in a GH-action context, especially since I have only seen them in that context at all. In all cases I know, a package manager would take care of resolving loosely specified versions. It would be nice if the workflow was that smart so using minor/major versions would be all up to the users and not the developers.

Anyway, WDYT about having the branches all in a folder e.g. release/v1 / release/v1.1?
I think creating a tag from a minor release branch would leave us with the tag overwriting again, in case of a post-release.
As for changing the default branch to v1, IMHO this would complicate things if the was to be a v2 release and for users, it would basically be the same as currently pointing to master.

@webknjaz
Copy link
Member

webknjaz commented Feb 8, 2021

Anyway, WDYT about having the branches all in a folder e.g. release/v1 / release/v1.1?
I think creating a tag from a minor release branch would leave us with the tag overwriting again, in case of a post-release.

Yeah, that's a good idea. Except I'd maybe call those rolling-release or something else explicitly pointing out that the "version" is actually a floating pointer to some moving target.

As for changing the default branch to v1, IMHO this would complicate things if the was to be a v2 release and for users, it would basically be the same as currently pointing to master.

Fair enough.

Comment on lines +12 to +15
- name: Set up Python 3.8
uses: actions/setup-python@v2
with:
python-version: 3.8
Copy link
Member

Choose a reason for hiding this comment

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

I guess it's time to bump this

Suggested change
- name: Set up Python 3.8
uses: actions/setup-python@v2
with:
python-version: 3.8
- name: Set up Python 3.9
uses: actions/setup-python@v2
with:
python-version: 3.9

@webknjaz
Copy link
Member

@s-weigand so I was thinking it may be a good idea to have branches like unstable/v2 potentially having more commits than release/v2 that should point to the latest v2.*.* tag (or release/v2.* for that matter). Any contributions would go through unstable/v2 and on release, it would be tagged with v2.*.*, and then release/v2 and release/v2.* would be fast-forwarded to that tag.

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.

2 participants