Feature/build scripts update #20
Conversation
* Set Helm chart default value for Enclave CPU advertisement to `false` to be in sync with kubernetes manifest
dpdornseifer
force-pushed
the
feature/build_scripts_update
branch
from
c783c9f to
12f8444
Compare
| main() { | ||
| ecr_login | ||
|
|
||
| docker manifest create --amend $ECR_URL/$IMAGE \ |
There was a problem hiding this comment.
If I read this correctly then this will expand to $ECR_URL/aws-nitro-enclaves-k8s-device-plugin:v0.2-x86_64 when run on an x86 machine. I do not think this is what we actually want. (?)
Probably better to stay with $ECR_URL/$REPOSITORY_NAME as it was before.
There was a problem hiding this comment.
arch got removed from image tag. Image just points at the generic image name + version tag now: https://github.com/aws/aws-nitro-enclaves-k8s-device-plugin/pull/20/files#diff-b23204e92f793e233cf28605758e6f1d4849830577492c85f00593f977527e27R17
scripts/create_manifest_docker.sh
Outdated
| #!/bin/bash | ||
| # Copyright 2022 Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| set -e |
There was a problem hiding this comment.
is -e actually what we want in this particular script, given that we do have handling through die et.al?
| tag_and_push_docker_image() { | ||
| local arch=$1 | ||
|
|
||
| docker tag $IMAGE-$arch $ECR_URL/$IMAGE-$arch |
There was a problem hiding this comment.
If I read common.sh correctly $IMAGE already contains the architecture. So are we creating images of name foo:bar-arch-arch? Do we actually want to do that?
There was a problem hiding this comment.
$(arch)' got removed from image to allow multi architecture build and tagging of images, -arch-arch` type of situations should be avoided under all circumstances
There was a problem hiding this comment.
Sorry, Somehow I was convinced to have looked at all the commits, but did in fact only look at the initial 3...
There was a problem hiding this comment.
Either way, that change to the variables in common should probably be put together with the changes to the user of these variables.
nit: The title of commit 4 is masking that quite a few things are happening apart from formatting. Ideally I'd split the formatting part from any functional changes to make reasoning about changes easier.
scripts/push_docker.sh
Outdated
| #!/bin/bash | ||
| # Copyright 2022 Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| set -e |
There was a problem hiding this comment.
Similar to above, I am not quite sure why we need -e here given the handling through die et al.
* Added _docker suffix to docker build scripts * Added multi architecture build to build_docker.sh script * Added multi architecture build handling to push_docker.sh
* Added Helm package and push scripts * Added utility functions to common.sh
* Fixed public ecr functions * Introduced HELM registry related variables * Added docker token reuse with 10s timeout * Made generic user confirmation function confirm() zsh compatible * Formatted common.sh
* Added `pipeline.sh` script for docker and Helm buid, packaging and release orchestration * Added `validate_artifacts_versions.sh` script to ensure that k8s, docker and Helm version tags are in sync when releasing
dpdornseifer
force-pushed
the
feature/build_scripts_update
branch
from
12f8444 to
218d2e8
Compare
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.