Use krane to fetch the project's SDK
#411
Conversation
This drops the requirement to manually keep track of the env var count for the static array.
cbgbt
force-pushed
the
twoliter-0.5.2-rc1
branch
from
2659352 to
6cb224c
Compare
cbgbt
force-pushed
the
twoliter-0.5.2-rc1
branch
from
6cb224c to
74b99eb
Compare
| ${KRANE} pull "${TLPRIVATE_SDK_IMAGE}" /dev/stdout --platform "${SDK_PLATFORM}" \ | ||
| | docker load |
There was a problem hiding this comment.
Not sure if it applies here, but there's potential for a performance hit writing to a pipe vs. a seekable file, if krane would otherwise be able to transfer parts in parallel.
This is fine, but I would probably write it to a temp file under the build directory, and register a cleanup function, vs. relying on /dev/stdout. Then there'd be clearer separation between "failed to pull" and "failed to load".
| echo "Pulling SDK '${TLPRIVATE_SDK_IMAGE}'" | ||
| ${KRANE} pull "${TLPRIVATE_SDK_IMAGE}" /dev/stdout --platform "${SDK_PLATFORM}" \ | ||
| | docker load | ||
| if [[ ${PIPESTATUS[0]} -ne 0 ]]; then |
There was a problem hiding this comment.
nit:
| if [[ ${PIPESTATUS[0]} -ne 0 ]]; then | |
| if [[ "${PIPESTATUS[0]}" -ne 0 ]]; then |
| patches.sort(); | ||
|
|
||
| for patch in patches { | ||
| println!("Executing `patch -p1 -i '{}'`", patch.display()); |
There was a problem hiding this comment.
Do we need to arrange to install patch on the image used for GitHub actions, or is it already there?
There was a problem hiding this comment.
Hmm, we may need to make sure that the cross build containers have patch. It seems that the GitHub actions succeeded, but I can explicitly install the dependency as well.
| fn main() { | ||
| let script_dir = env::current_dir().unwrap(); | ||
| let out_dir = PathBuf::from(env::var("OUT_DIR").unwrap()); | ||
|
|
||
| println!("cargo::rerun-if-changed=../build-cache-fetch"); | ||
| println!("cargo::rerun-if-changed=hashes/crane"); | ||
| println!("cargo::rerun-if-changed=patches"); |
There was a problem hiding this comment.
Interesting, I didn't realize cargo could watch an entire directory like this!
cbgbt
force-pushed
the
twoliter-0.5.2-rc1
branch
from
74b99eb to
fbf3bbb
Compare
|
Added Will resolve the rest of the feedback in an RC2 commit. |
Description of changes:
This PR resolves two issues:
fetch-sdktask useddocker pullto pull the SDK image. We usekranefor authenticated OCI registry interactions elsewhere in twoliter, so using it forfetch-sdkallows us to avoid needing to configure additional credentials for the docker daemon.kraneuses a dated version of theecr-loginpackage from awslabs/amazon-ecr-credential-helper. This pulls in the latest version, which should have support for the latest AWS endpoints.Testing done:
These tests were conducted on an EC2 instance with a role that allows pulling from the given private ECR repository.
Using the old twoliter, I removed all docker credential configurations and then attempted to use a build with an SDK overridden to a private ECR repository:
Using the new twoliter:
Logs from
kraneare also verbose with debug logs enabled:Terms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.