-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Generate + Sign bundle CI steps to accompany CDK changes #35
Conversation
Signed-off-by: jonahjon <[email protected]>
Signed-off-by: jonahjon <[email protected]>
Signed-off-by: jonahjon <[email protected]>
Signed-off-by: jonahjon <[email protected]>
Signed-off-by: jonahjon <[email protected]>
Signed-off-by: jonahjon <[email protected]>
Signed-off-by: jonahjon <[email protected]>
Signed-off-by: jonahjon <[email protected]>
Signed-off-by: jonahjon <[email protected]>
Signed-off-by: jonahjon <[email protected]>
Signed-off-by: jonahjon <[email protected]>
Signed-off-by: jonahjon <[email protected]>
Signed-off-by: jonahjon <[email protected]>
Signed-off-by: jonahjon <[email protected]>
Signed-off-by: jonahjon <[email protected]>
Signed-off-by: jonahjon <[email protected]>
Signed-off-by: jonahjon <[email protected]>
Signed-off-by: jonahjon <[email protected]>
Signed-off-by: jonahjon <[email protected]>
Signed-off-by: jonahjon <[email protected]>
Signed-off-by: jonahjon <[email protected]>
Signed-off-by: jonahjon <[email protected]>
Signed-off-by: jonahjon <[email protected]>
if [ ! -d "/root/.docker" ]; then | ||
mkdir -p /root/.docker | ||
fi | ||
mv generatebundlefile/scripts/docker-ecr-config.json /root/.docker/config.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this stuff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is for codecommit, so it can correctly authenticate with the account's reops.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's maybe add some comments to this file, explaining:
- What this is doing
- Why it's doing it
- When should you, as a developer, be doing any of this (or why you shouldn't)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment in the shell script about this, since can't add to the JSON file itself, as well as a link to the github project.
Signed-off-by: jonahjon <[email protected]>
Signed-off-by: jonahjon <[email protected]>
if [ ! -d "/root/.docker" ]; then | ||
mkdir -p /root/.docker | ||
fi | ||
mv generatebundlefile/scripts/docker-ecr-config.json /root/.docker/config.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's maybe add some comments to this file, explaining:
- What this is doing
- Why it's doing it
- When should you, as a developer, be doing any of this (or why you shouldn't)
Signed-off-by: jonahjon <[email protected]>
Signed-off-by: jonahjon <[email protected]>
Signed-off-by: jonahjon <[email protected]>
Signed-off-by: jonahjon <[email protected]>
Signed-off-by: jonahjon <[email protected]>
Signed-off-by: jonahjon <[email protected]>
@@ -102,16 +114,17 @@ helm push cert-manager-v1.5.3-4ae27c6a1df646736d9e276358d0a6b2daf99f55.tgz "oci: | |||
# sha256:a507b9e9e739f6a2363b8739b1ad8f801b67768578867b9fae7d303f9e8918e8 | |||
|
|||
# Populate test cases | |||
helm pull oci://public.ecr.aws/l0g8r8j6/eks-anywhere-test --version v0.1.1-4280284ae5696ef42fd2a890d083b88f75d4978a-helm | |||
mv eks-anywhere-test-v0.1.1-4280284ae5696ef42fd2a890d083b88f75d4978a-helm.tgz eks-anywhere-test-v1.0.1-helm.tgz | |||
public.ecr.aws/j0a1m4z9/eks-anywhere-packages --version 0.1.2-5010d89023bc2cdc520395b48d354c80ce2ad831-helm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get that this is a hack/dev file, but this is still gonna barf.
@@ -27,23 +27,24 @@ BASE_DIRECTORY=$(git rev-parse --show-toplevel) | |||
chmod +x ${BASE_DIRECTORY}/generatebundlefile/bin/generatebundlefile | |||
|
|||
# Faster way to install Cosign compared to go install github.com/sigstore/cosign/cmd/[email protected] | |||
curl -s https://api.github.com/repos/sigstore/cosign/releases/latest \ | |||
COSIGN_URL=$(curl -s https://api.github.com/repos/sigstore/cosign/releases/latest \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine, I guess, but if you have access to the jq
program, and you probably do, then something like this might be better:
curl -s https://api.github.com/repos/sigstore/cosign/releases/latest \
| jq -r '.assets[].browser_download_url | select(endswith("cosign-linux-amd64"))'
${BASE_DIRECTORY}/bin/oras push -u AWS -p "${ECR_PASSWORD}" "${IMAGE_REGISTRY}/eks-anywhere-packages-bundles:v1" bundle-1.20.yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: This exposes the password to anyone running ps
at the time. Not the end of the world, but not a good habit. Consider instead:
aws ecr-public get-login-password --region us-east-1 \
| oras login --username AWS --password-stdin "${IMAGE_REGISTRY}"
oras push "${IMAGE_REGISTRY}/eks-anywhere-packages-bundles:v1" bundle-1.20.yaml
The reason for the separate login
command is that the push
command doesn't accept --password-stdin
sadly, though I imagine that will be added at some point.
This way, the password is passed via stdin, and isn't present as a program argument for anyone to see via the ps
command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only run in Codebuild, I tried doing something similar to up above but wasn't able to get it working due to some docker wackiness. I can try again probably in another PR though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created an issue to come to later #53
Signed-off-by: jonahjon <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ewollesen, jonahjon The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Issue #, if available:
Description of changes:
Merge before CDK Changes.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.