so tldr, when a PR comes in nothing will happen, after you check it and you want to test it you as a

so tldr, when a PR comes in nothing will happen, after you check it and you want to test it you as a maintainer can add the label

ok-to-test

ok-to-test

ok-to-test

ok-to-test to the PR, then the actions kick in

j0rgeOP•2/28/23, 4:49 AM

then it makes and pushes a pr-27pr-27

j0rgeOP•2/28/23, 4:50 AM

or whatever the PR is

Jj0rge tldr it was designed to not allow us to do that on purpose for security reasons,...

Bigpod•2/28/23, 10:35 AM

it makes sense for them to not allow pushes from PRs action runs, that could be a nightmare

Jj0rge so tldr, when a PR comes in nothing will happen, after you check it and you want...

Robert•2/28/23, 11:46 AM

Even with this, you've got to take into account that anybody can change the workflows in their PRs. If you give PRs permissions to push to GHCR, you're also giving permissions for PRs to push images with the :latest tag.

The safest way would be to require approval on all commits, and not manage this through a workflow.
https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks

GitHub Docs

Approving workflow runs from public forks - GitHub Docs

Marco (马可)•2/28/23, 11:47 AM

Workflow changes only take effect after merging to main

Marco (马可)•2/28/23, 11:47 AM

We tested this yesterday to confirm

Robert•2/28/23, 11:53 AM

Ah, you've already enabled the setting that all workflows require approval
https://github.com/ublue-os/experimental/pull/21

Marco (马可)•2/28/23, 11:56 AM

Heh, the experimental repos GHA doesn’t build on PR

Marco (马可)•2/28/23, 11:56 AM

It also limits only for first time contributors

Marco (马可)•2/28/23, 11:57 AM

We also are using an “ok-to-build” label system - but that only works if you can’t modify it during PR

Robert•2/28/23, 11:57 AM

Please can you try approving this workflow
https://github.com/ublue-os/experimental/actions/runs/4292680387

Marco (马可)•2/28/23, 11:58 AM

Well

Marco (马可)•2/28/23, 11:59 AM

This didn’t work last night

Marco (马可)•2/28/23, 11:59 AM

This whole saga is probably one of the more frustrating things I’ve come across in recent memory

Marco (马可)•2/28/23, 12:00 PM

Guess we’ll need to use that additional button for approving an action run

Bigpod•2/28/23, 12:01 PM

Honestly i would have seperate workflow for prs

Bigpod•2/28/23, 12:02 PM

That just doesnt push

Marco (马可)•2/28/23, 12:02 PM

That means we cant rebase to test pr easily

Marco (马可)•2/28/23, 12:02 PM

It’s a huge bonus for reviewers to have a VM to validate changes IRL

Robert•2/28/23, 12:06 PM

I know it's not ideal, but unless you're planning on using AWS ECR or some other service with fine-grained access controls, I don't see how you can limit this.
With ECR, you can "assume" roles only from specific branches, and each role can only create images with specific tags.
So a

production

production

role is only assumable by the

main

main

branches, and this role can create latestlatest tags.
A developmentdevelopment role is assumable by all branches, and can create pr-*pr-* tags.

Or just keep the approval button for all forked workflow runs/don't push in PRs.

Marco (马可)•2/28/23, 12:23 PM

I think approval button for now

Marco (马可)•2/28/23, 12:23 PM

Doesn’t need to be perfect just needs to work and we can figure out and iterate over time

j0rgeOP•2/28/23, 1:02 PM

ok so what are we doing?

Robert•2/28/23, 1:04 PM

I think create a PAT which can write to packages, then require all workflows to be approved by a maintainer.
The PAT will be used during the docker logindocker login step, and allow PRs to push once a maintainer has approved the code is not malicious.

j0rgeOP•2/28/23, 1:05 PM

ok so an approval button instead of adding a label?

Robert•2/28/23, 1:07 PM

By requiring approval, i mean this.

The label wouldn't work as the logic in the workflow that checks if the label exists can be edited by any PRs created from forks.

j0rgeOP•2/28/23, 1:08 PM

oh, ok that looks fine to me, we're on the middle setting now, which is what it defaulted to

Robert•2/28/23, 1:08 PM

At the minute, that's fine since there's no PAT which allows pushing to GHCR from forks

j0rgeOP•2/28/23, 1:08 PM

all the docs we've been reading are recommending against a PAT

j0rgeOP•2/28/23, 1:08 PM

I guess that's the old way to do it?

Robert•2/28/23, 1:09 PM

Using GHCR, I think it's the only way. But you're only using the PAT on workflows you've already checked, so it's much safer than allowing all PRs to use the PAT

Bigpod•2/28/23, 1:10 PM

on github actions if you are using GHCR you shouldnt use a PAT

Robert•2/28/23, 1:11 PM

But we can't use the default GITHUB_TOKEN since it doesn't have permissions on forked workflows.
What other options are there?

j0rgeOP•2/28/23, 1:31 PM

we just need a way to test stuff

j0rgeOP•2/28/23, 1:31 PM

like herm's pr I want to test in a VM to test all the flatpak installation, etc.

Jj0rge like herm's pr I want to test in a VM to test all the flatpak installation, etc.

Bigpod•2/28/23, 1:54 PM

yea its a dilema

RRobert But we can't use the default GITHUB_TOKEN since it doesn't have permissions on f...

Marco (马可)•2/28/23, 1:54 PM

The current workflow we have fixes that

Marco (马可)•2/28/23, 1:55 PM

which is a pull_request that pushes to an artifact on the run, then a second run that executes when that workflow completes, using the artifact that was uploaded to publish

Marco (马可)•2/28/23, 1:55 PM

@j0rge I'm going to dig back in and finish up what we were doing last night

Marco (马可)•2/28/23, 1:55 PM

If we use the above require approval for all outside contributors we can remove the need to use PR labels

Marco (马可)•2/28/23, 2:20 PM

Actually, I guess using ECR for PR pushes and GHCR for prod would be good (or any other registry)

j0rgeOP•2/28/23, 3:24 PM

@marcoceppi I hadn't thought about that "Require approval" setting in github, I think if we had that we wouldn't want the label

Marco (马可)•2/28/23, 3:24 PM

Yeah, I think we can use that and drop the label

j0rgeOP•2/28/23, 5:05 PM

@herm @marcoceppi tested PR-79 in a vanilla VM and everything worked perfectly!

Marco (马可)•2/28/23, 5:06 PM

@j0rge you can approve and merge

j0rgeOP•2/28/23, 5:06 PM

yeah on it

j0rgeOP•2/28/23, 5:08 PM

https://github.com/ublue-os/base/blob/main/Containerfile

GitHub

base/Containerfile at main · ublue-os/base

Base image: Silverblue with unfiltered Flathub, distrobox, and automatic updates - base/Containerfile at main · ublue-os/base

j0rgeOP•2/28/23, 5:08 PM

oh yeah

j0rgeOP•2/28/23, 5:08 PM

much cleaner now too

so tldr, when a PR comes in nothing will happen, after you check it and you want to test it you as a

Similar Threads

Similar Threads

Similar Threads