Inject default values to container image name to avoid CI failure in from-fork PRs #108
Conversation
Oded-B
changed the title
Oded-B
requested review from
hnnsgstfssn,
Freyert,
ashvarts,
yzdann and
jared-logan-patrick-ct
Oded-B
changed the title
There was a problem hiding this comment.
I was trying to figure this out, but wasn't really able to find exactly why it was happening.
I'm not sure we should inject commercetools/telefonistka as a default value - seems like it shouldn't be necessary to me, but again I also cannot point to why 🤷
Since we've pinned to commits of docker/build-push-action, docker/metadata-action and docker/login-action, I thought an upgrade could potentially resolve it, but also not sure. I think we should still upgrade the versions to @v6, @v5 and @v3 respectively either way but let's do separately.
| @@ -75,7 +75,7 @@ jobs: | |||
| uses: docker/metadata-action@906ecf0fc0a80f9110f79d9e6c04b1080f4a2621 | |||
| with: | |||
| images: | | |||
| ${{ env.REGISTRY }}/${{ env.IMAGE_NAME }} | |||
| ${{ env.REGISTRY != '' && env.REGISTRY || 'ghcr.io' }}/${{ env.IMAGE_NAME != '' && env.IMAGE_NAME || 'commercetools/telefonistka'}} | |||
There was a problem hiding this comment.
Do you know why we need to inject commercetools/telefonistka as a fallback value hardcoded here?
Could we just check if the value is set and skip or something if it is not?
I don't understand why our check would fail, regardless of values on other forks - we have the value set!
There was a problem hiding this comment.
Maybe GH doesn't provide these env variables to GH workflows generated from a fork as security mechanism? so people couldn't sniff your secrets by manipulating CI/CD config and opening a PR?
There's already a mechanism to that which requires maintainer approval for workflows coming from new contributors, and these keys are not secrets, just plain "variables", but maybe it's a "defence in depth" kind of thing 🤷
There was a problem hiding this comment.
Written right here on the top of the Variable page in GH UI :)
There was a problem hiding this comment.
Yeah that I understand, but I don't understand why it's running in the context of the fork when the PR is opened in this repository 😕
There was a problem hiding this comment.
Since these are not sensitive values, may I suggest defining them directly in the workflow instead at the top level?
There was a problem hiding this comment.
Do you mean just hardcode the value to
images: |
ghcr.io/ommercetools/telefonistka
?
I'm ok with that.
The initial thought behind this was to allow forks to override these values so they can build images in their own repos without maintaining different CI/CD configurations. I'm not if anyone cares about that.
@hnnsgstfssn ?
There was a problem hiding this comment.
defining them directly in the workflow instead at the top level
I'm also on board with that.
I'm not if anyone cares about that.
I think we focus on what's needed in the context of this repository - I still don't understand why there are complications with forks 🤷 let's leave this discussion and we'll see if it comes up again.
There was a problem hiding this comment.
Perhaps you can just enforce ghcr.io and instead of using the image name you use the github context github.repository which translates to <org>/<repo-name>. That way forks can still create these and have pushed to their own org automatically.
I agree that we should review our usage of I want to unblock #106 and #105, are you ok with the proposed solution? |
Sure let's go ahead and we'll address anything else that comes up after. |
Description
Workflows run from forks don't have access to
env.*variables so they fail the container build stage with:This pr will inject
commercetools/telefonistkavalues for those cases while still supporting forks overriding their image names.Type of Change
Checklist