Add a flag to control pushing new tags on unchanged images #1112

zbarahal · 2019-08-27T21:46:38Z

My team ran into a use case where due to Bazel pushing a new tag to ECR for each service every time the monorepo changes we ran into the 100 tag per image cap on ECR for services that don't change often. This is because ECR groups tags onto images by digest so if your image isn't changing but you keep pushing new tags you end up with a bunch of tags on one image.

Per #1106 I added a new flag to the new_push functionality and updated the pusher. I may explore adding this to the old fast pusher as well.

Closes #1106

googlebot · 2019-08-27T21:46:47Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

zbarahal · 2019-08-27T21:49:30Z

@smukherj1 and I are working through some testing questions on the linked issue. I opened the PR so they could view my WIP. Barring any issues with testing I believe this PR is done but there may be changes.

smukherj1 · 2019-08-28T13:32:05Z

@zbarahal Thanks for the PR. Could you please sign the CLA? Also, we would also like to include a test for the changed behavior in this PR as well. See my comment here for instructions on how to fix the issue you were seeing with new_container_push.

zbarahal · 2019-08-28T17:25:05Z

@smukherj1 thanks for the help. I'm working on getting our organization CLA signed (should be done today) so that's the holdup. I'll add the tests in the meantime.

I voiced this on the issue but do you have a timeline for the release of this rewrite? Depending on that timeline I may try to add this to the Python pusher as well.

zbarahal · 2019-08-28T18:01:34Z

@smukherj1 is there somewhere I should add integration tests for all of this? From reading the codebase I don't understand how the tests are currently working. I cam definitely write a Go test for the new pusher functionality I added but I'm not sure if there's a spot that tests the Bazel code.

zbarahal · 2019-08-28T18:09:07Z

Okay I think I understand it now actually. Looks like a composite of shell scripts + bazel targets in e2e.sh. I'll write something up

smukherj1 · 2019-08-28T19:28:41Z

@zbarahal suggest following #580 for status of the python -> Go migration. Don't have a timeline yet because it can change based on issues/incompatibilities discovered during the migration. I'm actively working on the migration right now.

zbarahal · 2019-08-28T21:42:41Z

@smukherj1 Integration tests are added and CI is passing. The PR suggests adding @alex1545 as a reviewer but since we have been discussing it should you be the reviewer? Either way is fine by me but this is ready for review now.

Still working on the CLA but we'll have that done soon.

zbarahal · 2019-08-28T21:46:24Z

There are also a lot of CI builds attached to this PR and I'm not sure which are required. Travis is passing and I've cleaned up the formatting for buildifier. Let me know if other CIs are failing that are required.

smukherj1

Thanks a lot ! The change mostly looks good! Just a few minor comments.

smukherj1 · 2019-08-29T13:30:17Z

container/go/cmd/pusher/pusher.go

@@ -37,6 +38,7 @@ var (
 	clientConfigDir = flag.String("client-config-dir", "", "The path to the directory where the client configuration files are located. Overiddes the value from DOCKER_CONFIG.")
 	legacyBaseImage = flag.String("legacyBaseImage", "", "Path to a legacy base image in tarball form. Should be specified only when format is legacy.")
 	configPath      = flag.String("configPath", "", "Path to the image config. Should be specified only when format is legacy.")
+	onlyPushChanged = flag.Bool("only-push-changed", false, "If set to true, will only push images where the digest has changed.")


Suggest naming this skip-on-unchanged-digest to make it more explicit what defines the "change".

smukherj1 · 2019-08-29T13:31:42Z

container/go/cmd/pusher/pusher.go

@@ -126,6 +130,27 @@ func main() {
 	log.Printf("Successfully pushed %s image from %s to %s", *format, imgSrc, stamped)
 }

+// Checks whether an images digest exists in a repository


// digestExists checks whether an ...

Also change images to image's

Also, our coding style is to have Go comments end with a period, i.e., be complete sentences.

smukherj1 · 2019-08-29T13:39:22Z

container/go/cmd/pusher/pusher.go

+	if err != nil {
+		return false, errors.Wrapf(err, "unable to get local image digest")
+	}
+	digestRef, err := name.NewDigest(strings.Join([]string{dst, digest.String()}, "@"))


I feel name.NewDigest(fmt.Sprintf("%s@%s", dst, digest)) is more readable.

smukherj1 · 2019-08-29T13:40:22Z

container/go/cmd/pusher/pusher.go

+	}
+	digestRef, err := name.NewDigest(strings.Join([]string{dst, digest.String()}, "@"))
+	if err != nil {
+		return false, errors.Wrapf(err, "Couldn't create ref from digest")


Couldn't -> couldn't

Our convention for error messages added to an error object is to begin with small letters and not end in a period or any other punctuation

smukherj1 · 2019-08-29T13:45:44Z

container/go/cmd/pusher/pusher.go

+	}
+	remoteImg, err := remote.Image(digestRef, remote.WithAuthFromKeychain(authn.DefaultKeychain))
+	if err != nil {
+		if strings.HasPrefix(err.Error(), ManifestUnknownError) {


Suggest using the this here.

container/go/cmd/pusher/pusher.go

smukherj1 · 2019-08-29T13:47:11Z

container/new_push.bzl

@@ -179,6 +182,11 @@ new_container_push = rule(
            mandatory = True,
            doc = "The label of the image to push.",
        ),
+        "only_push_changed": attr.bool(


Suggest naming this skip-on-unchanged-digest as well.

smukherj1 · 2019-08-29T13:47:36Z

container/new_push.bzl

@@ -179,6 +182,11 @@ new_container_push = rule(
            mandatory = True,
            doc = "The label of the image to push.",
        ),
+        "only_push_changed": attr.bool(
+            default = False,
+            mandatory = False,


mandatory = False is the default so this is not necessary

smukherj1 · 2019-08-29T13:48:23Z

container/go/cmd/pusher/pusher.go

@@ -46,6 +48,8 @@ const (
 	manifestFile = "manifest.json"
 	// indexManifestFile is the filename of image manifest config in OCI format.
 	indexManifestFile = "index.json"
+	// ManifestUnknownError is the error returned when a digest is not found
+	ManifestUnknownError = "MANIFEST_UNKNOWN"


Can be removed after addressing another comment below.

smukherj1 · 2019-08-29T13:52:02Z

testing/e2e.sh

+  cd "${ROOT}"
+  clear_docker_full
+  cid=$(docker run --rm -d -p 5000:5000 --name registry registry:2)
+  EXPECT_CONTAINS "$(bazel run @io_bazel_rules_docker//tests/container:new_push_test_only_push_changed_unchanged_tag_1 2>&1)" "Successfully pushed docker image"


Suggest looking for the message that indicates the image push is being skipped here and the next line. Note that this log message doesn't currently exist and I suggested to add one in a separate comment.

smukherj1 · 2019-08-29T13:55:51Z

/gcbrun

zbarahal · 2019-08-29T17:08:52Z

@googlebot I signed it!

zbarahal · 2019-08-29T18:25:06Z

/gcbrun

smukherj1 · 2019-08-29T18:27:46Z

/gcbrun

smukherj1 · 2019-08-29T19:03:25Z

@googlebot rescan

googlebot · 2019-08-29T19:03:35Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

k8s-ci-robot · 2019-08-29T19:06:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: smukherj1, zbarahal

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:

~~OWNERS~~ [smukherj1]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

zbarahal · 2019-08-29T19:07:09Z

Thanks @smukherj1!

…d#1112) * Add flag to check if digest already exists before pushing a new tag * Clean up formatting * Add comment for const * Use more generic form of error * Add integration tests for only_push_changed flag * Run buildifier * Fix dict ordering * More buildifier changes * Remove unnecessary mandatory field * Rename flag to be more clear as to purpose * Use existing error and clean up join * Use correct coding style for comments/errors * Add log message for not pushing * Update bazel deps for transport package * Fix import sort order * Remove bin that got checked in * Buildifier

zbarahal added 4 commits August 26, 2019 16:41

Add flag to check if digest already exists before pushing a new tag

c9e140a

Clean up formatting

20bcfa0

Add comment for const

a6559ba

Use more generic form of error

bddd92b

zbarahal requested review from alex1545, nlopezgi and smukherj1 as code owners August 27, 2019 21:46

googlebot added the cla: no label Aug 27, 2019

k8s-ci-robot added the size/M label Aug 27, 2019

zbarahal mentioned this pull request Aug 27, 2019

container_push skip tag if sha256 exists #1106

Closed

Add integration tests for only_push_changed flag

e3968e9

k8s-ci-robot added size/L and removed size/M labels Aug 28, 2019

zbarahal added 3 commits August 28, 2019 14:21

Merge branch 'master' into digest_check_push

f7a2cea

Run buildifier

c1839c6

Merge branch 'master' into digest_check_push

21381a6

Fix dict ordering

e76dd88

More buildifier changes

54d5a3b

smukherj1 reviewed Aug 29, 2019

View reviewed changes

zbarahal added 9 commits August 29, 2019 10:15

Remove unnecessary mandatory field

bc94d95

Rename flag to be more clear as to purpose

efdc020

Use existing error and clean up join

37a7fe2

Use correct coding style for comments/errors

aa52062

Add log message for not pushing

a5458ca

Update bazel deps for transport package

8ae743c

Fix import sort order

87855ad

Remove bin that got checked in

495c91c

Buildifier

85a1213

googlebot added cla: yes and removed cla: no labels Aug 29, 2019

k8s-ci-robot assigned smukherj1 Aug 29, 2019

smukherj1 approved these changes Aug 29, 2019

View reviewed changes

k8s-ci-robot added the lgtm label Aug 29, 2019

k8s-ci-robot added the approved label Aug 29, 2019

smukherj1 merged commit 5aedb19 into bazelbuild:master Aug 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a flag to control pushing new tags on unchanged images #1112

Add a flag to control pushing new tags on unchanged images #1112

zbarahal commented Aug 27, 2019

googlebot commented Aug 27, 2019

zbarahal commented Aug 27, 2019

smukherj1 commented Aug 28, 2019

zbarahal commented Aug 28, 2019

zbarahal commented Aug 28, 2019

zbarahal commented Aug 28, 2019

smukherj1 commented Aug 28, 2019

zbarahal commented Aug 28, 2019

zbarahal commented Aug 28, 2019

smukherj1 left a comment

smukherj1 Aug 29, 2019

smukherj1 Aug 29, 2019

smukherj1 Aug 29, 2019

smukherj1 Aug 29, 2019

smukherj1 Aug 29, 2019

smukherj1 Aug 29, 2019

smukherj1 Aug 29, 2019

smukherj1 Aug 29, 2019

smukherj1 Aug 29, 2019

smukherj1 commented Aug 29, 2019

zbarahal commented Aug 29, 2019

zbarahal commented Aug 29, 2019

smukherj1 commented Aug 29, 2019

smukherj1 commented Aug 29, 2019

googlebot commented Aug 29, 2019

k8s-ci-robot commented Aug 29, 2019

zbarahal commented Aug 29, 2019

Add a flag to control pushing new tags on unchanged images #1112

Add a flag to control pushing new tags on unchanged images #1112

Conversation

zbarahal commented Aug 27, 2019

googlebot commented Aug 27, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

zbarahal commented Aug 27, 2019

smukherj1 commented Aug 28, 2019

zbarahal commented Aug 28, 2019

zbarahal commented Aug 28, 2019

zbarahal commented Aug 28, 2019

smukherj1 commented Aug 28, 2019

zbarahal commented Aug 28, 2019

zbarahal commented Aug 28, 2019

smukherj1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smukherj1 commented Aug 29, 2019

zbarahal commented Aug 29, 2019

zbarahal commented Aug 29, 2019

smukherj1 commented Aug 29, 2019

smukherj1 commented Aug 29, 2019

googlebot commented Aug 29, 2019

k8s-ci-robot commented Aug 29, 2019

zbarahal commented Aug 29, 2019