-
-
Notifications
You must be signed in to change notification settings - Fork 671
Fix creation of releases #40840
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
base: develop
Are you sure you want to change the base?
Fix creation of releases #40840
Conversation
8069f9f
to
a619c02
Compare
Documentation preview for this PR (built with commit f2b0b0b; changes) is ready! 🎉 |
You do not restore the code you removed, which was introduced by the PR #39194. Did you consider the problem the PR solved to prepare the present PR? |
This seems unnecessary since we already have https://github.com/sagemath/sage/releases which is specifically for releases and where people would reasonably expect to find releases. I'd prefer if we didn't duplicate information unnecessarily and kept release announcements in the standard place for releases on GitHub. I also think that we could be making better use of GitHub Discussions than we are now, and I think this would clutter it with unnecessary announcements. I don't know enough about the build and release process to comment on the rest of this, I'll leave that for others. |
Do you mean to say that the current PR does not do the generation of release notes, Anyway, #39194 had some very verbose code to do something which looks like a rather standard GitHub Action to upload a build artifact. Actually, a more mainstream |
If you think there is a benefit to announcing releases on GitHub Discussions to be discussed there (would this replace or be in addition to the sage-release mailing list?) then I'd prefer a dedicated category for this like "Releases" instead of putting it in the general announcements. |
@tobiasdiez - would you add a call to https://github.com/marketplace/actions/release-notes-action here, or in a followup? |
oops, sorry, it's actually looking fine. There is |
No.
#39194 solved the problem raised in sagemath/website#480 (comment). To solve the problem, #39194 replaced the action Then #40709 replaced the "very verbose code" with
The present PR, as a sequel to #40709, should present a solution to both problems (sagemath/website#480 (comment) |
There are discussions going on. @vincentmacri has some questions, not answered yet. @dimpase Why did you give "positive review" prematurely? |
@kwankyu sorry, you are not helpful here. Why do you force everyone to manually dig up what exactly you meant to solve in that old PR? You don't value other people time. Anyhow I don't see your problem as a problem. Yes, all PRs merged in a stable release are listed, including ones merged in prereleases. |
@kwankyu this is good as is. If you want to improve, open a follow up PR. |
I recommend you to merge #40843 to test your PR. |
Thanks for testing. (that this fails is expected as I'm running it as part of this PR not via a tag) |
needs: [package_tarballs, sdists] | ||
runs-on: ubuntu-latest | ||
if: github.ref_type == 'tag' | ||
permissions: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
I don't like your idea of using the sagemath repo itself as a playground or for experiments. Experiment the idea on your repo. Then if you are convinced that it is a good idea, then officially propose it to sage-devel. On the other hand, I like the idea of replacing sage-release with the Github discussions. But we need to hear from the community and especially @vbraun before officially launching it. I suggest to remove the "create discussion" part from this PR. |
Test with #40843 https://github.com/kwankyu/sage/actions/runs/17826513537 failed. Please merge the branch of #40843 to your branch, so that I don't need to resolve merge conflicts any more. Then I will close #40843. |
8d1f1d5
to
b056757
Compare
Looks like you forgot to set the secret. |
.github/workflows/release.yml
Outdated
token: ${{ github.ref_type == 'tag' && secrets.RELEASE_CREATION_TOKEN || 'FAIL' }} | ||
generate_release_notes: true | ||
prerelease: ${{ contains(github.ref_name, 'beta') || contains(github.ref_name, 'rc') }} | ||
discussion_category_name: announcements |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discussion_category_name: announcements |
I'm not opposed to replacing the sage-release mailing list like you suggested, but I agree with @kwankyu that there should be a discussion about it first on sage-devel.
If we do decide to announce releases on GitHub Discussions, I would really prefer that we have a dedicated "Release Announcements" category and keep the regular "Announcements" category for more substantial/important announcements (otherwise people will likely mute the announcements channel and miss important announcements).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for testing it here for a few weeks before bringing it to sage-devel, I think looking at the conky repo you linked which does this is sufficient for getting a feel for it. My feeling from looking at https://github.com/brndnmtthws/conky/discussions/categories/general is that the automated posts to a category that has other uses will flood the category and make it hard to find more important discussions (which is why I would want a dedicated category for this).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can create a dedicated category (perhaps "Releases" or "Release Announcements") when sage-devel decides to switch to github discussion.
I don't know if posting release announcements to a dedicated category is possible, by the way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can create a dedicated category (perhaps "Releases" or "Release Announcements") when sage-devel decides to switch to github discussion.
I don't know if posting release announcements to a dedicated category is possible, by the way.
In the workflow @tobiasdiez gives the name of the category to post to, so it should just be a matter of changing that to whatever you name the discussion category for release announcements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've now renamed the category to "Release". Since it's easy to just delete the category with all it's posts again, this should give a suitable testing env. But if you feel this should be discussed on sage-devel before testing it out in the sage repo, may I ask you to open such a discussion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will post an announcement after this PR is merged, that releases are now also on GH discussions and that people are invited to try to post/interact there and that after some testing phase we can decide to replace sage-release or to end the experiment.
I don't know what to write before this PR is merged, as there is nothing for people to see or try out yet. So what was your idea to post now to sage-devel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will post an announcement after this PR is merged, that releases are now also on GH discussions and that people are invited to try to post/interact there and that after some testing phase we can decide to replace sage-release or to end the experiment.
I worry that this will give the impression to the community that switching to GH discussion is already implemented and we are forcing the community to adopt it.
I agree with: "... looking at the conky repo you linked which does this is sufficient for getting a feel for it."
I don't know what to write before this PR is merged, as there is nothing for people to see or try out yet. So what was your idea to post now to sage-devel?
Just ask the community to look around the conky repo, and propose replacing the sage-release to release announcements in our own github discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion, but I still think it's better to give people an actual preview of how it would look like instead of a copy in some random, unrelated repo. Especially since this preview very simple to implement and can be reverted easily.
I also don't have time at the moment to write something suitable for sage-devel...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vbraun Your opinion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My opinion is that these notes are only needed for real releases.
The secret is already used in the previous step. Anyway, would you merge #40843 to this PR to facilitate testing? |
Then it is likely that there is a problem with using the secret as introduced in #40654 (which was never tested, right?) I've now removed the need of a secret by simply triggering the website workflow directly after creating a release. This worked well: https://github.com/tobiasdiez/sage/actions/runs/17846322248 (the 401 error in the final trigger website workflow step is expected as I don't have the rights to do this) with release https://github.com/tobiasdiez/sage/releases/tag/10.11 and discussion tobiasdiez#16 |
As far as I remember, #40654 was tested with the secret. I just checked my own repo that the secret is not there any more. Perhaps it was expired. I created the secret again at my repo. I am testing again: https://github.com/kwankyu/sage/actions/runs/17846547780 Let's see. |
I am testing your branch merged with #40843. Please merge it to your branch to make things easy. |
OK. With the added secret, it works: https://github.com/kwankyu/sage/actions/runs/17846547780 See also the created release: https://github.com/kwankyu/sage/releases/tag/100.1.beta35 |
As a member of the Code of Conduct Committee, I am restoring the disputed label. Please note that this means that the approach that we agreed upon in sage-devel applies here now. |
Why do you remove |
OK. You moved it. Looks okay. But that is another thing to watch to see if it works well. |
sagemathgh-40843: Restore release notes creation step <!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> fixes the issue discussed in https://groups.google.com/g/sage- devel/c/LFfhN0CpGJ8. However, we retain the idea of sagemath#40709 that a github release is made only in one step in the workflow. Previously, before sagemath#40709, a github release was made in two steps "Create release" and "Create release assets". This strategy worked well but started failing some months ago. Test (with sagemath#40840): https://github.com/kwankyu/sage/actions/runs/17846547780. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#40843 Reported by: Kwankyu Lee Reviewer(s):
As was pointed out on sage-devel, the creation of the release stopped working.
It appears there are actual multiple problems:
All of these issues are fixed as follows:
As a small bonus a new Announcment at https://github.com/sagemath/sage/discussions/categories/announcements is posted for each release. Also the workflow is renamed to "release" to make it easier to find.
📝 Checklist
⌛ Dependencies