-
Notifications
You must be signed in to change notification settings - Fork 503
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
[VDF] anago: Publish container images on K8s Infra (k8s-staging-kubernetes) #1199
Conversation
@justaugustus: Adding label: Reasons for blocking this PR:[Changes to certain release tools can affect our ability to test, build, and release Kubernetes. This PR must be explicitly approved by SIG Release repo admins.] Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Needs kubernetes/k8s.io#673. |
287bdc8
to
8df5dc4
Compare
730edd2
to
66d3eed
Compare
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
lib/releaselib.sh
Outdated
arches=($(ls "$release_images")) | ||
for arch in "${arches[@]}"; do |
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.
you can do:
for arch in ${release_images}/*; do
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 went with:
mapfile -t arches < <(find "${release_images}" -maxdepth 1 -mindepth 1 -type d -exec basename {} \;)
for arch in "${arches[@]}"; do
based on the guidance in shellcheck SC2207: https://github.com/koalaman/shellcheck/wiki/SC2207
Using find
and basename
takes some of the unexpected *
behavior out of the equation.
# @param version - version tag | ||
# @param build_output - build output directory | ||
# @return 1 on failure | ||
release::docker::validate_remote_manifests () { |
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 know in other places in this file it's not being checked but when you are writing new one you should check if the function receives enough arguments it expects and if not fail.
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.
Thanks! I've added common::argc_validate
to both functions. :)
lib/releaselib.sh
Outdated
if ! skopeo inspect "docker://${image}:${version}" --raw >/dev/null 2>&1; then | ||
logecho "Could not find manifest list for ${image}:${version}" | ||
return 1 | ||
else | ||
manifest=$(skopeo inspect "docker://${image}:${version}" --raw) | ||
fi |
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.
Have you tried:
manifest=$(skopeo inspect "docker://${image}:${version}" --raw >/dev/null 2>&1)
if [ $? ]; then
logecho "Could not find manifest list for ${image}:${version}"
return 1
fi
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 opted for a bit of a combination of the two:
if ! manifest=$(skopeo inspect "docker://${image}:${version}" --raw); then
logecho "Could not find manifest list for ${image}:${version}"
return 1
fi
See the guidance on shellcheck SC2181: https://github.com/koalaman/shellcheck/wiki/SC2181
cdda749
to
b59d4af
Compare
Here we add 'release::docker::validate_remote_manifests()', a function which validates the existence of image manifests on a remote registry by doing the following: - Using 'skopeo inspect' to retrieve an image manifest - Looping through the architectures and checking for an architecture-specific digest within the manifest using jq We do some minor cleanup by using 'find' and 'basename' to populate arrays for 'for' loops. Additionally, we validate the correct amount of arguments for 'release::docker::validate_remote_manifests()' and 'release::docker::release()' using 'common::argc_validate'. This function closely resembles 'release::docker::release()' and could probably be consolidated. We make the decision to not do this refactor here to minimize the changes to surrounding functions. Signed-off-by: Stephen Augustus <saugustus@vmware.com>
@bartsmykla -- Thanks for the great review! I've implement some of your suggestions. :) |
Successfully tested this earlier today...
|
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
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: cpanato, hasheddan, justaugustus, saschagrunert 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 |
What type of PR is this?
/kind feature
What this PR does / why we need it:
As part of the vanity domain flip, we need to be able to publish container images on K8s Infra.
As a domain restriction organization policy is in place on the Google Infra projects, we cannot
completely move staging to K8s Infra until dl.k8s.io is moved as well (both the redirect AND the GCS bucket contents).
So in the meantime, we will instead run the GCB stage/release jobs within the current
kubernetes-release-test
GCP project and grant its GCB service account access to write container images into the newk8s-staging-kubernetes
staging project.(Part of the larger effort to move to the stage/release process to K8s Infra: #911)
Special notes for your reviewer:
Blocked on kubernetes/k8s.io#673.Does this PR introduce a user-facing change?