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

Bump version #656

Merged
merged 1 commit into from Nov 23, 2020
Merged

Bump version #656

merged 1 commit into from Nov 23, 2020

Conversation

camilonova
Copy link
Member

No description provided.

@codecov
Copy link

codecov bot commented Nov 23, 2020

Codecov Report

Merging #656 (5ec16da) into master (9529d68) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #656   +/-   ##
=======================================
  Coverage   73.51%   73.51%           
=======================================
  Files          29       29           
  Lines        1688     1688           
=======================================
  Hits         1241     1241           
  Misses        447      447           

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 9529d68...5ec16da. Read the comment docs.

CHANGES.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@claudep claudep left a comment

Choose a reason for hiding this comment

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

I think that ideally, the changelog items should point to the issue or commit number. Apart from that, this looks good!

Copy link
Member

@jezdez jezdez left a comment

Choose a reason for hiding this comment

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

LGTM!

@camilonova camilonova merged commit 0a0309e into master Nov 23, 2020
@camilonova camilonova deleted the v12.0.7 branch November 23, 2020 15:41
@camilonova
Copy link
Member Author

@jezdez it did not show up at jazzband releases. Can you take a look?

@jezdez
Copy link
Member

jezdez commented Nov 23, 2020

@camilonova Ah right, I think there is some weirdness around GitHub releases versus Git tags. When we were on Travis-CI it was enough to release when a tag was created, for GitHub Actions (and the way I've seen it more often configured) the condition under which a release process is triggered is when a "GitHub release" is published.

I just checked on https://github.com/jazzband/sorl-thumbnail/releases and saw the 12.7.0 not as a "release" but just as a "tag", clicked "Edit" and then "Publish release". This should be rolling out now. The release workflow ran now: https://github.com/jazzband/sorl-thumbnail/runs/1442977373?check_suite_focus=true#step:9:13

I'm currently wondering how to handle this, whether we should try to find a solution like we had with Travis-CI where a Git tag was enough to trigger the release process.

@hugovk I know you've had some experience with this, can you shed some light on the topic of GitHub releases versus Git tags?

@hugovk
Copy link
Member

hugovk commented Nov 23, 2020

Ah yes, have a look at https://github.com/jazzband/prettytable/blob/master/RELEASING.md, you need to go to https://github.com/jazzband/prettytable/releases and create a "GitHub release" from the Git tag.

And then deploy.yml is triggered by that GitHub release:

  release:
    types:
      - published

That workflow is good for that project because it's using Release Drafter to generate release notes in the GitHub release. And in general it's good to create a GitHub release because that notifies people who are watching releases on the repo.


It may be possible to trigger from a Git tag instead. Edit: https://futurestud.io/tutorials/github-actions-run-a-workflow-when-creating-a-tag

Or I have seen GitHub Actions to automatically create a GitHub release from Git tag. Edit: https://github.com/actions/create-release

@jezdez
Copy link
Member

jezdez commented Nov 23, 2020

Yeah, the tricky part is that I can't disable either the releases web UI or git tag pushes, so both use cases need to be supported for Jazzband I'm afraid. I'm a bit wary to let project leads decide on one way to choose since Jazzband's open door policy would still allow bad actors to ignore and create the tag on their own. And having different workflow setups for all the projects sounds like a maintenance problem.

For the case in which a GitHub release is created when a tag is pushed, I think that could lead to a problem when someone were to create a release, which in effect creates the tag and would trigger the release creation again, right? Unless I could check if there is already a release with that name?

@hugovk
Copy link
Member

hugovk commented Nov 23, 2020

For the case in which a GitHub release is created when a tag is pushed, I think that could lead to a problem when someone were to create a release, which in effect creates the tag and would trigger the release creation again, right?

I don't think so: you can only create a single release for any given tag. For example, this test repo already has a 0.2 release. Attempting to create a new release with the same version shows:

Ignore, click publish, error:

@jezdez
Copy link
Member

jezdez commented Nov 23, 2020

Right, but would the create-release action that you linked above when triggered from a tag push event fail if there would already be a tag? I think so.

So I think I'd like to lean towards just listening to pushed tags instead of published releases for triggering the Jazzband release process by default. And project leads can make the optional decision to additionally use release-drafter etc and listen to the "release published" event instead.

@hugovk
Copy link
Member

hugovk commented Nov 23, 2020

Good idea, sounds sensible to default to the simpler method, especially to match the Travis way.

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.

None yet

4 participants