Build on all supported platforms in standard CI run#478
Conversation
adamtroiani
requested review from
andrewhassan
and removed request for
jeffcharles
andrewhassan
left a comment
andrewhassan
left a comment
There was a problem hiding this comment.
Overall this is looking pretty good! I just have a question about the organization of the workflows.
.github/workflows/publish.yml
Outdated
| # Upload all .gz files | ||
| find artifacts -name "function-runner-*-${{ inputs.tag_name || github.event.release.tag_name }}.gz" -type f | while read file; do | ||
| gh release upload ${{ inputs.tag_name || github.event.release.tag_name }} "$file" | ||
| done | ||
|
|
||
| # Upload all .gz.sha256 files | ||
| find artifacts -name "function-runner-*-${{ inputs.tag_name || github.event.release.tag_name }}.gz.sha256" -type f | while read file; do | ||
| gh release upload ${{ inputs.tag_name || github.event.release.tag_name }} "$file" | ||
| done |
There was a problem hiding this comment.
Instead of having to find the artifacts here, which requires knowledge of what the build step is doing internally, I wonder if we could have the build step output the list of the artifacts it generated using output parameters.
There was a problem hiding this comment.
I simplified the asset upload process so that it doesn't use find artifacts anymore and instead simply looks at the artifacts directory. Let me know if the use of output parameters is still necessary.
adampetro
left a comment
adampetro
left a comment
There was a problem hiding this comment.
Overall looks great, just have one question
| - name: Archive assets | ||
| run: gzip -k -f ${{ matrix.path }} && mv ${{ matrix.path }}.gz ${{ matrix.asset_name }}.gz | ||
|
|
||
| - name: Upload assets to artifacts | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: ${{ matrix.asset_name }}.gz | ||
| path: ${{ matrix.asset_name }}.gz | ||
|
|
||
| - name: Generate asset hash | ||
| run: ${{ matrix.shasum_cmd }} ${{ matrix.asset_name }}.gz | awk '{ print $1 }' > ${{ matrix.asset_name }}.gz.sha256 |
There was a problem hiding this comment.
I think the condition would apply to these steps too?
There was a problem hiding this comment.
Yes you're right - just added the condition to those steps as well
adampetro
left a comment
adampetro
left a comment
There was a problem hiding this comment.
Looks great! Can you squash down the commits into one before/during merging?
3 participants
Refactor GitHub Actions workflows to use reusable workflow
Changes
build.ymlthat handles building binaries for all platformsResolves shop/issues-shopifyvm#291