Source binaries for linux artifacts from docker images #4491
Conversation
azdagron
requested review from
evan2645,
amartinezfayo,
MarcosDY and
rturner3
as code owners
azdagron
force-pushed
the
build-artifacts-from-docker
branch
from
c2467f9
to
33eb9b1
Compare
This gives us static binaries linked against musl for our release artifacts, unifying our libc dependency for both docker and non-docker and simplifying our build tooling. Since artifact building is now fairly complicated and really only part of the CI/CD pipeline, got rid of the Makefile target for it. Fixes: spiffe#4346 Signed-off-by: Andrew Harding <azdagron@gmail.com>
azdagron
force-pushed
the
build-artifacts-from-docker
branch
from
33eb9b1
to
62586ba
Compare
| - name: Load cached build tools | ||
| uses: actions/cache@88522ab9f39a2ea568f7027eddc7d8d8bc9d59c8 # v3.3.1 | ||
| - name: Install regctl | ||
| uses: regclient/actions/regctl-installer@b6614f5f56245066b533343a85f4109bdc38c8cc # main |
There was a problem hiding this comment.
Can we pin to a versioned commit? Looks like the latest version is v1.2.8. I see we are pinning to main in other places too, so not something we would need to handle in this PR.
I suspect we will also run into problems with dependabot with this dependency because it isn't generating GitHub releases. Perhaps we can create an issue in that project similar to msys2/setup-msys2#327.
There was a problem hiding this comment.
We tried to move the regctl stuff forward at some point but ran into some breaking changes. I think this is worth exploring at another time.
| OUTDIR=${OUTDIR:-"${REPODIR}/artifacts"} | ||
|
|
||
| TARCMD=tar | ||
| if [[ $(uname -s) == "Darwin" ]]; then |
There was a problem hiding this comment.
nit: Since this script is only invoked for Linux artifact builds, I think this can be removed. Otherwise if this is meant to handle macOS builds too, we should rename this file and fix the invocation in build_artifacts.sh to handle both macOS and Linux.
There was a problem hiding this comment.
This was mostly for test convenience. As a developer it was convenient to build the linux artifacts from my box instead of spinning up yet-another-container. I'm ok removing it 🤷
There was a problem hiding this comment.
I'm inclined to keep it as is to make the developer experience easier.
There was a problem hiding this comment.
In that case, it makes sense to keep, but since it's just for dev purposes, I would suggest adding a comment to clarify that.
| command -v regctl >/dev/null 2>&1 || { echo -e "The regctl cli is required to run this script." >&2 ; exit 1; } | ||
| command -v "${TARCMD}" >/dev/null 2>&1 || { echo -e "The ${TARCMD} command is required to run this script." >&2 ; exit 1; } | ||
|
|
||
| build_artifact amd64 |
There was a problem hiding this comment.
nit: just for readability, could define a supported_archs array at the top of the script and loop through it here
There was a problem hiding this comment.
that's what the old code did and i found it less readable putting in all the looping stuff and array declaration to handle two elements :)
| copy_binary_from_multiarch_tar "$ARCH" "spire-agent" "${STAGING}/bin" | ||
| copy_binary_from_multiarch_tar "$ARCH" "oidc-discovery-provider" "${EXTRAS_STAGING}/bin" | ||
|
|
||
| mkdir -p "${OUTDIR}" |
There was a problem hiding this comment.
nit: can we consolidate some of these mkdir -p commands into a single command?
Signed-off-by: Andrew Harding <azdagron@gmail.com>
Signed-off-by: Andrew Harding <azdagron@gmail.com>
azdagron
force-pushed
the
build-artifacts-from-docker
branch
from
d42dd22
to
2653584
Compare
This gives us static binaries linked against musl for our release artifacts, unifying our libc dependency for both docker and non-docker and simplifying our build tooling.
Since artifact building is now fairly complicated and really only part of the CI/CD pipeline, got rid of the Makefile target for it.
Fixes: #4346