Workflow fixes

[AndrewFerr]

The most important fix is to the Synapse Guest Module release job.

[AndrewFerr]

TODO: This was supposed to make the Synapse build job run after the tests job. Ideally it should also start after the static analysis job.

[dbkr]

I think this one is probably for the server folk. We probably ought to fix the codeowners to assign appropriately.

[AndrewFerr]

@langleyd @t3chguy Should CI push images of the Synapse module on every run of the build job? Currently it pushes only on changes to main or when a secret is provided:

element-modules/.github/workflows/synapse-module.yml

Line 62 in 79e5859

push: ${{ github.event_name == 'push' && github.ref == 'refs/heads/main' || github.event_name == 'pull_request' && secrets.GH_APP_OS_APP_ID != '' }}

With that said, an image is always pushed for releases (which is what publish-release-synapse-module.yml is for).

[AndrewFerr]

If this is getting too complicated, I'm happy to split just the release job fix to its own PR.

[MTRNord]

@langleyd @t3chguy Should CI push images of the Synapse module on every run of the build job? Currently it pushes only on changes to main or when a secret is provided:

element-modules/.github/workflows/synapse-module.yml

Line 62 in 79e5859

push: ${{ github.event_name == 'push' && github.ref == 'refs/heads/main' || github.event_name == 'pull_request' && secrets.GH_APP_OS_APP_ID != '' }}

With that said, an image is always pushed for releases (which is what publish-release-synapse-module.yml is for).

For some historic context, from nordeck POV: This was originally happening since we wanted to be able to deploy PR branches at times. And the secret was only accessible for nordeck and iirc dependabot or renovate (cant remember which one was used in this repo).

[t3chguy]

Should CI push images of the Synapse module on every run of the build job? Currently it pushes only on changes to main or when a secret is provided:

That secret doesn't exist here, we should not be relying on pushing with secrets in a PR unless we wire it up safely to also work on forked pull requests, inconsistent contribution experience is unfair and also really annoying to deal with as a reviewer. I think we should not be pushing on PRs, if you want the CI could upload the exported image as an artifact for local testing. Or go the full hog, escalate privileges via workflow_run and upload it to a secondary docker image, with the tag being the PR number, executing zero untrusted code in the workflow_run environment to ensure no secrets get leaked to nefarious contributors.

[AndrewFerr]

That secret doesn't exist here, we should not be relying on pushing with secrets in a PR unless we wire it up safely to also work on forked pull requests, inconsistent contribution experience is unfair and also really annoying to deal with as a reviewer.

Thanks for the explanation & suggestions. The secret is removed by 1001f7d.

I think we should not be pushing on PRs, if you want the CI could upload the exported image as an artifact for local testing. Or go the full hog, escalate privileges via workflow_run and upload it to a secondary docker image, with the tag being the PR number, executing zero untrusted code in the workflow_run environment to ensure no secrets get leaked to nefarious contributors.

For now I won't do this, since the image is simple enough to build & test with locally. But your suggestion is a good idea if anyone needs to use the image in CI runs.