Review 1: Comments and bashisms

Author: smarterclayton

pkg/image/prune/prune.go

@@ -230,8 +230,8 @@ func (*dryRunRegistryPinger) ping(registry string) error {
 //
 // The ImageDeleter performs the following logic:
 //
-// remove any image was created at least *n* minutes ago and is *not* currently
-// referenced by:
+// remove any image that was created at least *n* minutes ago and is *not*
+// currently referenced by:
 //
 // - any pod created less than *n* minutes ago
 // - any image stream created less than *n* minutes ago
@@ -260,7 +260,7 @@ func NewPruner(options PrunerOptions) Pruner {
 	if options.PruneOverSizeLimit != nil {
 		pruneOverSizeLimit = fmt.Sprintf("%v", *options.PruneOverSizeLimit)
 	}
-	glog.V(1).Infof("Creating image pruner with keepYoungerThan=%v, keepTagRevisions=%s, pruneOverSizeLimit=%s allImages=%t",
+	glog.V(1).Infof("Creating image pruner with keepYoungerThan=%v, keepTagRevisions=%s, pruneOverSizeLimit=%s, allImages=%t",
 		options.KeepYoungerThan, keepTagRevisions, pruneOverSizeLimit, options.AllImages)
 
 	algorithm := pruneAlgorithm{}
@@ -314,11 +314,7 @@ func addImagesToGraph(g graph.Graph, images *imageapi.ImageList, algorithm prune
 		glog.V(4).Infof("Examining image %q", image.Name)
 
 		if !algorithm.allImages {
-			if image.Annotations == nil {
-				glog.V(4).Infof("Image %q with DockerImageReference %q belongs to an external registry - skipping", image.Name, image.DockerImageReference)
-				continue
-			}
-			if value, ok := image.Annotations[imageapi.ManagedByOpenShiftAnnotation]; !ok || value != "true" {
+			if image.Annotations[imageapi.ManagedByOpenShiftAnnotation] != "true" {
 				glog.V(4).Infof("Image %q with DockerImageReference %q belongs to an external registry - skipping", image.Name, image.DockerImageReference)
 				continue
 			}

test/end-to-end/core.sh

@@ -105,7 +105,7 @@ os::cmd::try_until_success "curl --max-time 2 --fail --silent 'http://${DOCKER_R
 os::cmd::expect_success "curl -f http://${DOCKER_REGISTRY}/healthz"
 
 os::cmd::expect_success "dig @${API_HOST} docker-registry.default.local. A"
-registry_pod=$(oc get pod -n default -l deploymentconfig=docker-registry --template='{{(index .items 0).metadata.name}}')
+registry_pod="$(oc get pod -n default -l deploymentconfig=docker-registry --template='{{(index .items 0).metadata.name}}')"
 
 # Client setup (log in as e2e-user and set 'test' as the default project)
 # This is required to be able to push to the registry!
@@ -148,11 +148,11 @@ os::log::info "Ruby's testing blob digest: $rubyimageblob"
 os::log::info "Docker pullthrough"
 os::cmd::expect_success "oc import-image --confirm --from=mysql:latest mysql:pullthrough"
 os::cmd::expect_success "docker pull ${DOCKER_REGISTRY}/cache/mysql:pullthrough"
-mysqlblob="$(oc get istag -o go-template='{{range .image.dockerImageLayers}}{{if gt .size 1024.}}{{.name}},{{end}}{{end}}' "mysql:pullthrough" | cut -d , -f 1)"
+mysqlblob="$(oc get istag -o go-template='{{range .image.dockerImageLayers}}{{if gt .size 4096.}}{{.name}},{{end}}{{end}}' "mysql:pullthrough" | cut -d , -f 1)"
 # directly hit the image to trigger mirroring in case the layer already exists on disk
 os::cmd::expect_success "curl -H 'Authorization: bearer $(oc whoami -t)' 'http://${DOCKER_REGISTRY}/v2/cache/mysql/blobs/${mysqlblob}' 1>/dev/null"
 # verify the blob exists on disk in the registry due to mirroring under .../blobs/sha256/<2 char prefix>/<sha value>
-os::cmd::try_until_success "oc exec --context='${CLUSTER_ADMIN_CONTEXT}' -n default -p ${registry_pod} du /registry | tee '${LOG_DIR}/registry-images.txt' | grep '${mysqlblob:7:100}' | grep blobs"
+os::cmd::try_until_success "oc exec --context='${CLUSTER_ADMIN_CONTEXT}' -n default -p '${registry_pod}' du /registry | tee '${LOG_DIR}/registry-images.txt' | grep '${mysqlblob:7:100}' | grep blobs"
 
 os::log::info "Docker registry start with GCS"
 os::cmd::expect_failure_and_text "docker run -e REGISTRY_STORAGE=\"gcs: {}\" openshift/origin-docker-registry:${TAG}" "No bucket parameter provided"
@@ -533,7 +533,7 @@ os::cmd::expect_success 'oadm policy add-cluster-role-to-user system:image-prune
 os::cmd::try_until_text 'oadm policy who-can list images' 'system:serviceaccount:cache:builder'
 
 # run image pruning
-os::cmd::expect_success_and_not_text "oadm prune images --token=$(oc sa get-token builder -n cache) --keep-younger-than=0 --keep-tag-revisions=1 --confirm" 'error'
+os::cmd::expect_success_and_not_text "oadm prune images --token='$(oc sa get-token builder -n cache)' --keep-younger-than=0 --keep-tag-revisions=1 --confirm" 'error'
 
 # record the storage after pruning
 os::cmd::expect_success "oc exec -p ${registry_pod} du /registry > '${LOG_DIR}/prune-images.after.txt'"
@@ -543,16 +543,17 @@ os::cmd::expect_code "diff ${LOG_DIR}/prune-images.before.txt ${LOG_DIR}/prune-i
 
 # prune a mirror, external image that is no longer referenced
 os::cmd::expect_success "oc import-image nginx --confirm -n cache"
-nginxblob="$(oc get istag -o go-template='{{range .image.dockerImageLayers}}{{if gt .size 1024.}}{{.name}},{{end}}{{end}}' "nginx:latest" -n cache | cut -d , -f 1)"
+nginxblob="$(oc get istag -o go-template='{{range .image.dockerImageLayers}}{{if gt .size 4096.}}{{.name}},{{end}}{{end}}' "nginx:latest" -n cache | cut -d , -f 1)"
 # directly hit the image to trigger mirroring in case the layer already exists on disk
 os::cmd::expect_success "curl -H 'Authorization: bearer $(oc sa get-token builder -n cache)' 'http://${DOCKER_REGISTRY}/v2/cache/nginx/blobs/${nginxblob}' 1>/dev/null"
 # verify the blob exists on disk in the registry due to mirroring under .../blobs/sha256/<2 char prefix>/<sha value>
-os::cmd::try_until_success "oc exec --context='${CLUSTER_ADMIN_CONTEXT}' -n default -p ${registry_pod} du /registry | tee '${LOG_DIR}/registry-images.txt' | grep '${nginxblob:7:100}' | grep blobs"
+os::cmd::try_until_success "oc exec --context='${CLUSTER_ADMIN_CONTEXT}' -n default -p '${registry_pod}' du /registry | tee '${LOG_DIR}/registry-images.txt' | grep '${nginxblob:7:100}' | grep blobs"
 os::cmd::expect_success "oc delete is nginx -n cache"
 os::cmd::expect_success "oc exec -p ${registry_pod} du /registry > '${LOG_DIR}/prune-images.before.txt'"
-os::cmd::expect_success_and_not_text "oadm prune images --token=$(oc sa get-token builder -n cache) --keep-younger-than=0 --confirm --all --registry-url=${DOCKER_REGISTRY}" 'error'
+os::cmd::expect_success_and_not_text "oadm prune images --token='$(oc sa get-token builder -n cache)' --keep-younger-than=0 --confirm --all --registry-url='${DOCKER_REGISTRY}'" 'error'
+os::cmd::expect_failure "oc exec --context='${CLUSTER_ADMIN_CONTEXT}' -n default -p '${registry_pod}' du /registry | tee '${LOG_DIR}/registry-images.txt' | grep '${nginxblob:7:100}' | grep blobs"
 os::cmd::expect_success "oc exec -p ${registry_pod} du /registry > '${LOG_DIR}/prune-images.after.txt'"
-os::cmd::expect_code "diff ${LOG_DIR}/prune-images.before.txt ${LOG_DIR}/prune-images.after.txt" 1
+os::cmd::expect_code "diff '${LOG_DIR}/prune-images.before.txt' '${LOG_DIR}/prune-images.after.txt'" 1
 os::log::info "Validated image pruning"
 
 # with registry's re-deployment we loose all the blobs stored in its storage until now