-
-
Notifications
You must be signed in to change notification settings - Fork 63
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
Publish release checklist and automate release verification #758
Conversation
Additional notes/future scope:At this moment the workflow will run on all published releases, which is fine for stable releases, however, this will not work correctly for "pre-releases" (RCs/betas and such) and the workflow is likely to fail for those type of releases. As those will generally only happen for a new major release, I'm not too concerned about this for now, but it would be nice if the workflow would recognize that the "unversioned web" PHAR should always be the latest stable release, not an RC release. |
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 taking the time to share the release checklist, @jrfnl.
This PR looks good to me. I have left some non-blocking suggestions and questions as inline comments.
name: "${{ matrix.download_flavour }}: ${{ matrix.pharfile }}" | ||
|
||
steps: | ||
- name: Retrieve latest release info |
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.
This job performs steps that are also performed in the job below. Have you considered moving the repeated steps to a separate job that can be reused?
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'm not sure what you mean by "separate job that can be reused" ?
I did consider moving some steps to a reusable workflow, but that's not actually an option.
A job using a reusable workflow can't have extra steps aside from the ones in the reusable workflow itself, so it wouldn't allow for the other check steps.
The other option would be to create a composite action, but that would need a separate repo, which I think is completely over the top.
Also see: https://docs.github.com/en/actions/sharing-automations/avoiding-duplication
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 don't have much experience with GitHub Actions. On a quick search, I came across the two options you mentioned and started wondering if they could be used here. However, I was not aware of the limitations you mentioned.
The only other option that I considered that I'm not sure as well if it would work or not, is to create a job to run the following steps:
PHP_CodeSniffer/.github/workflows/verify-release.yml
Lines 43 to 62 in 76ca8be
steps: | |
- name: Retrieve latest release info | |
uses: octokit/[email protected] | |
id: get_latest_release | |
with: | |
route: GET /repos/PHPCSStandards/PHP_CodeSniffer/releases/latest | |
env: | |
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | |
- name: "DEBUG: Show API request failure status" | |
if: ${{ failure() }} | |
run: "echo No release found. Request failed with status ${{ steps.get_latest_release.outputs.status }}" | |
- name: Grab latest tag name from API response | |
id: version | |
run: | | |
echo "TAG=${{ fromJson(steps.get_latest_release.outputs.data).tag_name }}" >> "$GITHUB_OUTPUT" | |
- name: "DEBUG: Show tag name found in API response" | |
run: "echo ${{ steps.version.outputs.TAG }}" |
Then use needs
(https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions#jobsjob_idneeds) to make this new job run before verify-available-downloads
and verify-phive
. It seems that there is a way to share the latest version between different jobs: https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/passing-information-between-jobs
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 see what you mean, but for those two very simple steps (+ two debug steps), I think that would be overengineering and that would delay the builds unnecessarily with the needs
.
run: echo ${{ steps.asset_version.outputs.VERSION }} | ||
|
||
- name: Fail the build if the PHAR is not the correct version | ||
if: ${{ steps.asset_version.outputs.VERSION != steps.version.outputs.TAG }} |
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 wonder if we should include a message here outputting that there was a version mismatch and print both versions to make it easier to debug failures.
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 think that would be possible. Not sure it is really needed.
For the record: both version numbers are already printed out in "DEBUG" steps earlier on in the workflow, so debugging this shouldn't be hard as it is.
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.
That is fair
|
||
# Note: the `.` is in the command to make it work for both PHPCS as well PHPCBF. | ||
- name: Verify the PHAR is nominally functional | ||
run: php ${{ steps.source.outputs.FILE }} . -e --standard=PSR12 |
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.
Is there a reason to use the -e
parameter here? I'm asking as this parameter does not exist for phpcbf
? I wonder if it would be preferable to use a command that includes parameters that exist for both phpcs
and phpcbf
?
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.
Good question. The problem is that these workflows are being run in a virgin environment (we're not checking out the repo), so there are no PHP files which could be scanned, so this seemed like a workable solution which would a) at the very least run the PHAR file and b) give a 0
exit code in both cases.
If you have a suggestion for another command, I'd be happy to consider it.
(and I did already consider -h
(help), but felt that it bowed out a little too early and had no risk of a non-0
exit code, which is why I didn't go with that)
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.
Can we generate a trivial PHP file to check against? echo '<?php echo "Hello, World!\n";' > hello.php
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.
If you have a suggestion for another command, I'd be happy to consider it.
(and I did already consider -h (help), but felt that it bowed out a little too early and had no risk of a non-0 exit code, which is why I didn't go with that)
-h
is precisely what I had in mind, but I missed the limitations that you mentioned. I can't think of a better command.
Can we generate a trivial PHP file to check against? echo '<?php echo "Hello, World!\n";' > hello.php
That seems like a good approach if it is straightforward to implement.
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.
If you both don't mind, I'd like to test the workflow with the upcoming 3.11.2 release tomorrow. Happy to iterate on the exact command after that if either of you want to submit a PR for that.
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.
Sounds good to me.
|
||
### Majors only | ||
|
||
- [ ] Move old changelog entries to `CHANGELOG_OLD.md` file. |
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.
Where is this file located? I was not able to find it in the repository.
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.
It doesn't exist yet as there has not been a new major release since the repo was taken over.
- [ ] Prepare changelog for the release and submit the PR. - PR #xxx | ||
- Based on the tickets in the milestone. | ||
- Double-check that any issues which were closed by PRs included in the release, have the milestone set. | ||
- Compare with auto-generated release notes to ensure nothing is missed. |
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 don't know what the auto-generated release notes are. I'm sharing this here in case you think it should be clarified in the release checklist.
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.
Ah, let me guess, you don't do releases ?
If you go to a repo on which you have the authority to create a release....
➡️ Go to the owner/repo/releases
page
➡️ Click on the "Draft a new release" button
➡️ Enter a tag name for the release...
And you will see a new button become available:
If you click on it, it will give you a machine generated changelog based on PR titles (which is horrible in my opinion as PR titles are often not intended for end-users and a changelog should be written for end-users):
Does that make it clearer for you ?
IMO this does not need further clarification in the checklist as, even if someone has never seen this before, they will come across the button anyhow the first time they do a release and will know it from then on.
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.
Ah, let me guess, you don't do releases ?
It is been a couple of years since the last time that I did a GitHub release :-)
Does that make it clearer for you ?
Yes, thanks!
IMO this does not need further clarification in the checklist as, even if someone has never seen this before, they will come across the button anyhow the first time they do a release and will know it from then on.
👍
.github/release-checklist.md
Outdated
- [ ] Verify that the website regenerated correctly and that the phars can be downloaded. | ||
- [ ] Create a release & copy & paste the changelog to it. | ||
- [ ] Upload the unversioned PHAR files + asc files to the release | ||
- [ ] Announce the release in the discussions forum via checkbox |
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 don't understand what via checkbox
means in this context.
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.
.github/release-checklist.md
Outdated
|
||
## After Release | ||
|
||
- [ ] Update the version nr in `Config::VERSION` file to the _next_ (patch) version. |
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'm assuming this is in the master
branch. I wonder if it should be mentioned in the sentence.
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.
What other branch could it be done in ?
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 thought about mentioning the master
branch more as a reminder to whoever is going through the checklist to ensure they are making the change in the correct branch.
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.
Well, for now, master
is the only possible branch and prior art (commits after previous releases) can be used as an example.
I think this may need revisiting when #3 is being addressed and/or if the repo would be going to a different branching scheme later, but for now, I think there is not much opportunity for confusion here.
|
||
- [ ] Post on Mastodon about the release (official account). | ||
- [ ] Post on X about the release (official account). | ||
- [ ] Post on LinkedIn (personal account). |
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.
It's not something that needs to be addressed now, but this line might need adjustment in the future if someone other than you were to make a release as it mentions a personal account.
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.
Well, there is no account on LI for PHPCS as a project, as LI doesn't really accommodate that situation, so in that case, either the action item would be skipped or done from the account of whomever does the release, providing they have a LinkedIn account.
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.
Looks good. Thanks for putting this together, and for making it public.
While the actual releasing can not be fully automated (due to security concerns related to signing releases in a GHA workflow), there are a number of verification checks which are part of the release workflow, which _can_ be automated. These checks were previously done as manual spot-checks. With the new workflow, they will now be executed structurally and automatically whenever a release is published on GitHub. The checks which are automated via this workflow are as follows: * For the PHAR files which can be downloaded from the GH "releases" page, the (unversioned) PHAR files published on the GH Pages website and the versioned PHAR files published on the GH Pages website for Phive (but which can also be downloaded manually), the following checks will now be run automatically: - Is the PHAR file available and can it be downloaded ? - Is the ASC (detached signature) file available and can it be downloaded ? - Verify the PHAR file via the attestation (which was created when the PHAR was created for a tag). - Verify the PHAR file is GPG signed and the signature matches. - Verify the PHAR file is functional (simple command runs without problems). - Verify the version with which the PHAR file identifies itself is the expected version. * For Phive: - Install via Phive. This will automatically also check the PHAR file is signed and the signature matches. - Verify the Phive installed PHAR file via the attestation. - Verify the Phive installed PHAR file is functional (simple command runs without problems). - Verify the version with which the PHAR file identifies itself is the expected version. Note: these checks will only run for releases from this repo. If a fork of the repo would publish their own releases, the workflow would fail anyhow (as the releases wouldn't be published to the website, nor accessible via Phive), so may as well prevent the workflow from running altogether.
I've been using and fine-tuning this release checklist over the past year. As part of the efforts to document the processes and policies for this repo, I'm now publishing the checklist for transparency and to make this knowledge transferable. Note: some of the steps which this list previously included in my local copy have now been automated via the "verify-release" GH Actions workflow. These steps have been removed from this checklist. Fixes 32
bcffd4a
to
224a57f
Compare
Rebased and reorganized commits without any further changes. Merging once the build has passed. |
Description
GH Actions: automate release verification steps
While the actual releasing can not be fully automated (due to security concerns related to signing releases in a GHA workflow), there are a number of verification checks which are part of the release workflow, which can be automated.
These checks were previously done as manual spot-checks. With the new workflow, they will now be executed structurally and automatically whenever a release is published on GitHub.
The checks which are automated via this workflow are as follows:
Add release checklist
I've been using and fine-tuning this release checklist over the past year.
As part of the efforts to document the processes and policies for this repo, I'm now publishing the checklist for transparency and to make this knowledge transferable.
Note: some of the steps which this list previously included in my local copy have now been automated via the "verify-release" GH Actions workflow. These steps have been removed from this checklist.
Suggested changelog entry
N/A
Related issues/external references
Fixes #32