Conversation
jelly
left a comment
There was a problem hiding this comment.
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.
Thanks! I am okay with splitting out the guide and flathub workflows, that's actually nice to be able to fix them more easily.
| flathub: | ||
| environment: flathub | ||
| env: | ||
| COCKPITUOUS_TOKEN: ${{ secrets.COCKPITUOUS_TOKEN }} |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
We used this token before when creating a branch in our flathub fork, but that's as much as I know of its usage
| @@ -0,0 +1,118 @@ | |||
| name: Release on flathub | |||
There was a problem hiding this comment.
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.
| @@ -0,0 +1,72 @@ | |||
| name: Update guide from release | |||
There was a problem hiding this comment.
Splitting this out and re-running manually is okay for me.
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 |
| environment: flathub | ||
| env: | ||
| COCKPITUOUS_TOKEN: ${{ secrets.COCKPITUOUS_TOKEN }} | ||
| COCKPITUOUS_TOKEN: ${{ secrets.COCKPITUOUS_RELEASE_TOKEN }} |
There was a problem hiding this comment.
The repo privilege is too much, sorry. This token has leaked before. It's absodefinitively not necessary to have full control over all of cockpit-project's repositories just to create a PR in a foreign one. Personal access tokens are just not the right tool for this kind of action, and if GitHub makes it so difficult to create a foreign PR, then so be it -- it's just three clicks every two weeks, it's really not worth spending so much effort on that.
There was a problem hiding this comment.
@martinpitt this is a separate token though that should only be used within the workflow. Given GitHub is good at hiding secrets I don't see how we could leak it?
You say it's 3 clicks but we are quite bad at doing it anyway, which is why I've been pushing on this
There was a problem hiding this comment.
I feel like this has to move into a workflow on the flathub repo. That has enough privileges with the default GitHub token to create a PR.
There was a problem hiding this comment.
i.e. the on: push trigger feels right? On the cockpit.git side we push the branch, on the flathub repo side it creates/updates the PR?
There was a problem hiding this comment.
I don't think we can, it still requires permissions to be able to take branches or tags from other repos. Nor do I know how to make the workflow trigger if we have an update - we could use their update checker thing but that solves the version updates but leaves the version changelog unchanged.
We can discuss this in another PR though, for low left it as it was
|
@Venefilyn I resolved a few of my previous review threads, but some are still relevant. But honestly: this makes the release workflow a lot more complicated (over 200 lines!), and harder to track/review/understand. |
Sure does introduce more lines.. which I'm also a bit frustrated with. The composite action I can get rid of and instead use the steps in it within each workflow itself if that helps reduce complexity. I figured that it reduces complexion for just going over the workflow in general as it's a fairly nice wrapper. Most of the time we'd still only care about executing the release workflow through a tag, or by triggering each step manually |
|
@Venefilyn If we do this restructuring, then the get-release-assets composite action is right, otherwise it would be a lot of duplication. My pain is more about having to do all this stuff in the first place -- the information is right there in the primary release workflow, and it's a lot of bureaucracy to move it around. And at least the node-cache workflow should better stay where it is. Otherwise, there are still the threads above, plus the squashing. My honest opinion is: "this is a waste of time and unnecessary debugging hell, let's forget about it", but as I said I can be talked into splitting out the flathub workflow. I would just suggest to not also try to fix the flathub PR/cockpituous token issue in this PR, it's getting unwieldy (and that problem needs a wholly different approach IMHO) |
martinpitt
left a comment
There was a problem hiding this comment.
See above, forgot to set the status so that it drops from my review list.
3e56bc7 to
c0bd225
Compare
Makes it possible to execute each step manually without being dependent on the other. 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 <freya@venefilyn.se>
c0bd225 to
251a314
Compare
Squashed!
Removed Only issue I have is that it can be difficult to parse. |

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: