-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix docker build for workflow_call events #11603
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but gets me thinking about the best pattern for constructing tag names, see below.
.github/workflows/build.yml
Outdated
@@ -245,7 +245,7 @@ jobs: | |||
matrix: | |||
arch: ["arm", "arm64", "386", "amd64"] | |||
env: | |||
repo: ${{ github.event.repository.name }} | |||
repo: "${{ env.PKG_NAME }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This clearly works correctly in this workflow, I just wonder if the pattern could be confusing in other repos. The name PKG_NAME
doesn't necessarily map to repo
. The helloworld
app does use the repository name for its docker tags, but I'm also questioning that now because the mapping is coincidental for some repos rather than something we can always rely on.
I would be tempted here to either replace all the env.repo
template values in tags
below with env.PKG_NAME
, or else to just hard-code packer
in the tags. But maybe this is something better discussed as a group first, and updated in the helloworld
apps before trying to roll out a new pattern on the fly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Sam! I just updated this to use env.PKG_NAME
instead of env.repo
for the docker build job. Nightly releases aren't in use by any other teams yet -- this was created specifically for packer to have feature parity with existing code in packer-releases -- but there's an open PR in the HelloWorld project discussing adding this support across products. Feel free to comment there if you have suggestions!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 with the changes suggested; I'll follow up on where we might need to change this in our example repos.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. I left a small question just for my clarification but please feel free to merge when ready.
@@ -245,7 +245,6 @@ jobs: | |||
matrix: | |||
arch: ["arm", "arm64", "386", "amd64"] | |||
env: | |||
repo: ${{ github.event.repository.name }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me . To confirm the issue here is that this event information is not available via scheduled actions, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! That's it. On workflow_call events, the github.event.repository.name isn't available, so we need to reference something else.
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. |
Related to #11601 (comment), the same change needs to be made for the docker build job to work for nightly build/releases.