-
Notifications
You must be signed in to change notification settings - Fork 1.2k
ci: Revamp release workflow #22630
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: main
Are you sure you want to change the base?
ci: Revamp release workflow #22630
Conversation
jelly
left a comment
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 a fan of splitting this up, our current workflow has dependencies so everything is easily observable from our view.
The current workflow is fully re-runnable, we can delete the release and re-run the workflow successfully.
martinpitt
left a comment
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! I am okay with splitting out the guide and flathub workflows, that's actually nice to be able to fix them more easily.
| run: | | ||
| set -eux | ||
| # this is a shared repo, prefix with project name | ||
| TAG="${GITHUB_REPOSITORY#*/}-$(basename $GITHUB_REF)" |
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 doesn't work with workflow_dispatch. You can only manually run workflows on branches, not tags. So as soon as the main branch head isn't tagged, this creates nonsense. And if it is, and the tag was changed, re-running this job won't remove the old tag first. That would be fixable, but I really think we should avoid that.
IMHO the node-cache job better stays in release.yml.
| flathub: | ||
| environment: flathub | ||
| env: | ||
| COCKPITUOUS_TOKEN: ${{ secrets.COCKPITUOUS_TOKEN }} |
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 doesn't currently exist in bots.git or cockpituous.git, so that would have to be set up first. Someone apparently added COCKPITUOUS_TOKEN to the "flathub" env manually, but this really needs to live in CI. Do you know what's the origin of that token? (It's painful to log in as cockpituous, I didn't do that now)
| # this is needed so we can push to a different repository | ||
| fetch-depth: 0 | ||
|
|
||
| - uses: robinraju/release-downloader@v1 |
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 is untrusted code, not reviewed or pinned to a particular SHA, and the download isn't verified, thus it breaks the chain of trust from the current release workflow.
It also seems unnecessary. You already construct the DOWNLOAD URL further down below, so that just may as well happen in the job-global env: and this becomes a curl call.
| @@ -0,0 +1,118 @@ | |||
| name: Release on flathub | |||
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 be talked into accepting this for flathub. I'm admittedly nervous about it, as it loses the tight coupling of "single source of truth" of the computed checksum. But if someone can manipulate our tarball downloads after the fact, we are already in trouble, so I can stomach that. The release_tag is at least a manual input, good enough.
| FILE: ${{ fromJson(steps.release-asset.outputs.downloaded_files)[0] }} | ||
| run: | | ||
| set -x | ||
| echo "shasum=$(sha256sum ${FILE} | awk '{ print $1 }' )" >> "$GITHUB_OUTPUT" |
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 can then be absorbed into the previous step that calls curl
.github/workflows/release.yml
Outdated
| @@ -1,5 +1,6 @@ | |||
| name: release | |||
| on: | |||
| workflow_dispatch: | |||
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.
Hard no. It's super dangerous to re-run a release after it already partially succeeded.
| @@ -0,0 +1,72 @@ | |||
| name: Update guide from 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.
Splitting this out and re-running manually is okay for me.
.github/workflows/update-guide.yml
Outdated
| repository: cockpit-project/cockpit-project.github.io | ||
| ssh-key: ${{ secrets.DEPLOY_KEY }} | ||
|
|
||
| - uses: robinraju/release-downloader@v1 |
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.
See flathub PR, let's replace this with a curl call.
.github/workflows/update-guide.yml
Outdated
| tar --directory source --extract --strip-components=1 --file '${{ fromJson(steps.release-asset.outputs.downloaded_files)[0] }}' | ||
| ( | ||
| cd build | ||
| ../source/configure | ||
| make doc/guide/html/index.html | ||
| ) |
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 is currently broken, and also needs to be dropped and reworked after #22693, so let's block on that.
Makes it possible to execute each step manually without being dependent on the other. Signed-off-by: Freya Gustavsson <[email protected]>
The Cockpituous token we have doesn't have all the required scopes needed to create PRs from our repo to the flathub PR. We mentioned that the token is used in a few places where it is possible that the key could be linked. Since we require more permissions to be able to create a PR from Cockpit's repo to flathub's Cockpit Client repo we need a new token with permissions: - `repo`, - `read:org`, and - `gist` Once done and this is merged to main we should hopefully get automated PR creations from Cockpit to Flathub org. Related-to: https://github.com/cockpit-project/cockpit/actions/runs/19295842838/job/55177626160 See-also: https://cli.github.com/manual/gh_auth_login Signed-off-by: Freya Gustavsson <[email protected]>
Removes a previous dependency in favor of a curl wrapper that gets the cockpit tarball release and verifies it against a checksum if available. If checksum is not available it is still provided to flathub release by taking the checksum from the downloaded tarball. Signed-off-by: Freya Gustavsson <[email protected]>
90d6d31 to
3e56bc7
Compare
|
@martinpitt I removed the dependency in favor of a composable action ourselves (just to reduce duplication, but I can move it to steps if wanted) Can see run here, guide succeeds as I had commented out the |

This revamps the release workflow to be manually triggered if something goes bad without having to create a new release
Coincidentally, this made it possible to experiment with the flathub PR creation which now works as it should. I've created a secret env within
cockpit-project/cockpitalready so the workflow should function when merged.https://github.com/Venefilyn/cockpit/actions/runs/19749612709/job/56590267873
Successful run here with some things were commented out to prevent accidental pushes to cockpit-project, hence failing
node-cachehttps://github.com/Venefilyn/cockpit/actions/runs/20852299144
And for working flathub workflow you can see that here
https://github.com/Venefilyn/cockpit/actions/runs/19747486121/job/56584524019
Dependencies: