We could also just remove the list of branches to make every branch build no matter its name. Only `

We could also just remove the list of branches to make every branch build no matter its name. Only

live

live

gets published to GHCR. So that may be the most desirable?

ArcitecOP•5/16/23, 4:07 PM

I am pretty sure this https://i.imgur.com/7EW7t9D.png list of branches:branches: was the old-school "protect against push to GHCR" idea before I changed it to only push

live

live

AArcitec We could also just remove the list of branches to make every branch build no mat...

xyny•5/16/23, 4:08 PM

Well, I don't know. If there's some in-repo development on a custom branch. I guess I should start making my PRs from a custom repo too to not pollute the template?

Xxyny Well, I don't know. If there's some in-repo development on a custom branch. I gu...

ArcitecOP•5/16/23, 4:08 PM

I think removing branches:branches: filtering for the auto-build triggers would solve that. So that way every branch gets built no matter its name. We're exactly as safe as before, since the deploy/sign steps only run for

live

live

ArcitecOP•5/16/23, 4:09 PM

And the builds seem to happen in the PR sender's own home.

xyny•5/16/23, 4:10 PM

Sure, do it. I guess stressing GHA shouldn't cause any issues.

ArcitecOP•5/16/23, 4:11 PM

Hmm, you make a point actually. Do we WANT anything except

template

template

(main atm) and

live

live

to build? If we want all other branches to be non-building, I could just list the exact branches.

ArcitecOP•5/16/23, 4:11 PM

People can still trigger manual builds on all other branches by manually triggering the workflow (that's the

workflow_dispatch:

workflow_dispatch:

trigger).

ArcitecOP•5/16/23, 4:12 PM

So... let's list main, template and live.

xyny•5/16/23, 4:12 PM

Well, we don't exactly want-want it but it shouldn't cause any issues.

ArcitecOP•5/16/23, 4:12 PM

Yeah. This solution sounds good, I'll refactor the docs a little bit around it.

xyny•5/16/23, 4:13 PM

Great

Xxyny Great

ArcitecOP•5/16/23, 4:19 PM

I was just finished, but then I realized we may wanna do this:

- Pushes: Auto-builds on

live

live

main

main

and

template

template

. As planned.
- PRs: Auto-builds every branch, by removing the branches:branches: filter. This is what I just realized that we may need to do. Since it turned out that PRs run in the SOURCE repo/branch, not the target. So PRs from let's say branch: fix-foo -> merge into startingpoint:mainbranch: fix-foo -> merge into startingpoint:main would fail to run its GHA if we don't remove that filter. If PRs are working the way it seems.

ArcitecOP•5/16/23, 4:26 PM

@hermmm I found the issue. It's a bug that will affect every PR from old forks.

ArcitecOP•5/16/23, 4:27 PM

Let's say people forked a while ago and their

main

main

contains the old workflow file.

That file is not properly setup to allow builds of branches.

So they make a branch fix-somethingfix-something and submit it to you.

Their old

main

main

branch workflow won;'t build fix-somethingfix-something and therefore nothing happens. 0 actions are listed.

ArcitecOP•5/16/23, 4:27 PM

I fixed it by fast-forwarding my own

main

main

(old) to match the proposed feature branch, so that the action runs.

xyny•5/16/23, 4:28 PM

So PRs run the local

main

main

branches workflows?

ArcitecOP•5/16/23, 4:28 PM

Yes primary branch of the local fork workflow is what runs inside PRs. @j0rge may be very interested to hear this too.

ArcitecOP•5/16/23, 4:29 PM

I converted the PR to a draft to stop the merge while I edit a comment that was misleading regarding this fact in build.yml.

xyny•5/16/23, 4:30 PM

We'll just have to instruct potential contributors with this issue to update their main branches

Xxyny We'll just have to instruct potential contributors with this issue to update the...

ArcitecOP•5/16/23, 4:34 PM

Yeah I added a note about that in build.yml too. It's ready for final check/merge.

ArcitecOP•5/16/23, 4:36 PM

This discovery also gives us another good reason why forks should keep an untouched "upstream main branch". Haha.

xyny•5/16/23, 4:38 PM

True, 'xcept when working you'd probably want to set

live

live

as the main branch for it to be the one displayed when opening the repo

ArcitecOP•5/16/23, 4:39 PM

Oh yeah true, so they'd still have to rebase. Good thing my comment is phrased that way:

ArcitecOP•5/16/23, 4:39 PM

"primary branch" is mentioned, phew.

ArcitecOP•5/16/23, 4:42 PM

I was really surprised that PRs build using the fork's primary workflow. It feels very backwards, since that means upstream has no control over the workflow checks of PRs. I could then write a funny workflow that just succeeds instantly, and fool any upstream repo into accepting the "successful build".

ArcitecOP•5/16/23, 4:43 PM

But it makes some sense too, otherwise people could hog the upstream repo's build queue for their own PR spam.

ArcitecOP•5/16/23, 4:43 PM

To fix this, I really think GitHub should be using the fork's build queue + upstream's primary workflow. That'd be a good fix.

xyny•5/16/23, 4:43 PM

Yeah. But I'd've thought it at least used the divergent branch's workflow instead of the forks main branch's one

ArcitecOP•5/16/23, 4:43 PM

Yeah, that was the most nuts thing to me. Literally not even using the PR's own branch's workflow.

ArcitecOP•5/16/23, 4:45 PM

@hermmm

ArcitecOP•5/16/23, 4:45 PM

Got done just in time to watch some TV.

ArcitecOP•5/16/23, 4:45 PM

Ah I see your comment, will do that first!

AArcitec Ah I see your comment, will do that first!

xyny•5/16/23, 4:45 PM

Still got some complaints for ya!

ArcitecOP•5/16/23, 4:47 PM

Okay I'll wait and do those now.

Xxyny _Still got some complaints for ya!_

ArcitecOP•5/16/23, 4:47 PM

I noticed that the readme uses "container image" in other places. Should I call it (the mentioned line) "published image" or "published container image"?

AArcitec I noticed that the readme uses "container image" in other places. Should I call ...

xyny•5/16/23, 4:48 PM

I think container image is alright in the README, cause that's what it technically is. I think either is fine and I'd go with "published image" for the mentioned line.

ArcitecOP•5/16/23, 4:49 PM

Agreed, it reads more nicely too.

ArcitecOP•5/16/23, 4:51 PM

@hermmm The fix is pushed. Was there anything else that needs doing before TV?

AArcitec @hermmm The fix is pushed. Was there anything else that needs doing before TV? �...

xyny•5/16/23, 4:51 PM

You're good for TV!

ArcitecOP•5/16/23, 4:52 PM

Sweet, time to see episode 2 of The Wire!

Hope you have a nice evening in Finland too!

xyny•5/16/23, 4:52 PM

Everything is done and dusted, well, it never is, but regarding this PR yes.

AArcitec Sweet, time to see episode 2 of The Wire! 😄 Hope you have a nice evening in Fin...

xyny•5/16/23, 4:52 PM

Thanks, you too!

ArcitecOP•5/16/23, 4:56 PM

Last minute thought: I just realized why forks don't run upstream's workflow when submitting PRs. Because that'd be the reverse security exploit: Then the upstream could create malicious actions that run in every fork that submits a PR, using the fork's secrets/credentials.

Likewise, PRs don't run on the upstream/target's build machine either, because that'd allow PRs to run exploits on the target infrastructure.

So yeah @j0rge : All Pull requests in all our repos run with the GitHub Actions workflow of the primary branch of the fork, using the fork's secrets/environments, using the fork's build queue/runner.

Therefore, we need to remind people to sync their fork's primary branch's workflow with upstream whenever they submit PRs.

ArcitecOP•5/16/23, 4:57 PM

https://tenor.com/view/network-confused-john-travolta-wire-gif-5799547

Tenor

ArcitecOP•5/16/23, 4:57 PM

^ Now it's time for The Wire!

j0rge•5/16/23, 4:59 PM

yeah we can't run stuff on forks and PRs

j0rge•5/16/23, 4:59 PM

like at first we wanted to make per PR builds but to do that securely requires more infra so we tabled it (it's the last card allllll the way to the right on the board)

xyny•5/16/23, 5:06 PM

Now that you're here @j0rge , could you go ahead and rename

main

main

in startingpoint to

template

template

if that is possible, as I seem to lack the required permissions or knowhow to do it.

j0rge•5/16/23, 5:07 PM

nor me, but we're about to find out lol

xyny•5/16/23, 5:08 PM

Well it changed!

We could also just remove the list of branches to make every branch build no matter its name. Only `

Similar Threads

Similar Threads

Similar Threads