Build on all supported platforms in standard CI run by adamtroiani · Pull Request #478

Build on all supported platforms in standard CI run by adamtroiani · Pull Request #478 · Shopify/function-runner

Refactor GitHub Actions workflows to use reusable workflow

Changes

Created a new reusable workflow build.yml that handles building binaries for all platforms
Updated both standard CI and publish workflows to use this reusable workflow

Resolves shop/issues-shopifyvm#291

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this is looking pretty good! I just have a question about the organization of the workflows.

Comment on lines +34 to +42

		# 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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks great, just have one question

Comment on lines +71 to +81

		- 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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the condition would apply to these steps too?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes you're right - just added the condition to those steps as well

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Can you squash down the commits into one before/during merging?