chore: refactor the github actions has-secrets logic #26795
Conversation
mistercrunch
requested review from
villebro,
geido,
eschutho,
rusackas,
betodealmeida,
nytai,
craig-rueda,
john-bodley and
kgabryje
as code owners
This part worries me a bit. Each key is set separately by Apache Infra, and sometimes they are (or soon will be) secrets for disparate systems (e.g. Google, Github, OpenAI, etc). I don't think it's safe to assume that if one secret exists that the others are all good... it would be best to pass an array of secrets, and have them all validated by the shared action. |
|
Just to make sure I'm understanding, is this fixing the issue wherein the |
.github/workflows/docker.yml
Outdated
| needs: config | ||
| if: needs.config.outputs.has-secrets | ||
| needs: check-secrets | ||
| if: needs.check-secrets.outputs.has-secrets == 'true' |
There was a problem hiding this comment.
Not totally related with this PR, but I think it only makes sense to build and push to a local file on the workspace workflow while the PR is open, so this means we are supposed to not have secrets here. the ephemeral-env workflow is expecting the build dir
.github/workflows/docker.yml
Outdated
| needs: config | ||
| if: needs.config.outputs.has-secrets == 'true' | ||
| needs: check-secrets | ||
| if: needs.check-secrets.outputs.has-secrets == 'true' |
There was a problem hiding this comment.
should we always build images on PRs? like we were doing before? docker_build_push.sh will only push if DOCKERHUB_USER is present
also this is kind of tricky to test, except opening a similar PR directly to the repo
There was a problem hiding this comment.
Yes that's right, more work to do here
There was a problem hiding this comment.
About both your comments, I think I figured it out in #26801, where we always run docker, but in forks it runs without authentication, which means it builds an DOESNT push, just making sure that docker build works.
Now I was also able to improve caching there, where in forks, it's use the registry cache as input, but it cannot push the cache and won't. Unclear what the cache hit rate will be, but should be pretty good unless the PR touches dependencies. Also if you don't touch the frontend there's a lot savings on docker build time.
As docker cache can be really good if done well, it could be tempting to eventually run tests in-docker, and saving a lot on build times.
While working on #26772, I realized that the has-secret logic was broken for unclear reasons. Now after doing this fix, I looked and realized that there's similar logic across about a dozen other gh actions, and thought it'd be a good thing to refactor/fix this with a reusable action. Now many of these workflows are set to trigger on push-on-master only meaning it's less of an issue since a false positive on has-secret dpesn't matter in that context since there's always a secret. I still thought I should refactor this to something we can trust and build upon. The solution introduces this new simple reusable action. One minor note is in cases where we need multiple secrets, as in say DOCKERHUB_TOKEN and DOCKERHUB_USER, we simply look at one and assume that's clear-enough of an indicator.
mistercrunch
force-pushed
the
has_secrets
branch
2 times, most recently
from
315f4b3 to
d524461
Compare
mistercrunch
force-pushed
the
has_secrets
branch
from
103d1d8 to
beb9f13
Compare
|
Frustrated head-butting with GitHub reusable actions not working as expected. Tried a million things and couldn't make the output value work. Closing for now |
[reopening] While working on #26772, I realized that the has-secret logic was broken for unclear reasons. Now after doing this fix, I looked and realized that there's similar logic across about a dozen other gh actions, and thought it'd be a good thing to refactor/fix this with a reusable action.
Now many of these workflows are set to trigger on push-on-master only meaning it's less of an issue since a false positive on has-secret dpesn't matter in that context since there's always a secret.
I still thought I should refactor this to something we can trust and build upon.
The solution introduces this new simple reusable action.
One minor note is in cases where we need multiple secrets, as in say DOCKERHUB_TOKEN and DOCKERHUB_USER, we simply look at one and assume that's clear-enough of an indicator.