container_push: add skip_unchanged_digest attribute

[sluongng]

Bring push-all on par with container_push in term of feature parity.

Generally --skip-unchanged-digest is a very useful flag to have. One can simply setup a release pipeline upon successful CI to just do bazel run <push-all-target> to push all container images (wrapped in a container bundle) at once.

By checking against upstream registry whether an existing digest already existed, we will only push images which were updated as the result of recent changes introduced to the repo.

The alternative solution is to calculate the SCM diff (git-diff) of the repo to deduce which bazel target were affected, query for container_push targets and bazel run each target 1-by-1, which is complicated to setup (especially when handling deleted code).

---

This PR added the ability to verify unchanged digest to bundle push (contrib/push-all) rule for convenience.

Without this, the ability to skip unchanged digest is exclusive to container_push and one would need something like https://github.com/atlassian/bazel-tools/tree/master/multirun to achieve the equivalent.

Caveats: when push-all with a container bundle that has duplicated images and push-all is set to run in parallel, the final result might be random. Use sequential push option if you think this is an issue.

Closes #1741

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sluongng
To complete the pull request process, please assign smukherj1 after the PR has been reviewed.
You can assign the PR to them by writing /assign @smukherj1 in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

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

[sluongng]

I did split this PR into 3 separate commits for easier review.

This closes #1741

[sluongng]

/assign @smukherj1

[sluongng]

@alexeagle it would be nice to get this reviewed / merge 🙏

[zoidyzoidzoid]

Maybe splitting the commits into separate PRs will make it easier / safer to merge?

[pcj]

Thanks @sluongng this looks like a useful addition. I think it will need some documentation about the behaviour as it may surprise people when upgrading.

[sluongng]

Thanks @sluongng this looks like a useful addition. I think it will need some documentation about the behaviour as it may surprise people when upgrading.

@pcj hey, regarding documentation improvement, do you have any recommendation?

I look back at the change and see that:

• This is not anyway breaking, all the current functionality remains to be the same
• New feature skip_unchanged_digest was added with a default value set to False (disable) and accompanied with starlark doc.

Reviewing https://github.com/bazelbuild/rules_docker/search?q=push-all there is not a lot of other references that needed to be updated. 🤔 Am I missing a generated markdown somewhere?

[sluongng]

I just force pushed the PR to update according to feedback from Slack.

Changes from last round:

1. Drop the cleanup commits, will be sending them as a separate PR.

The diff is now expected to be small without noise.

[sluongng]

@alexeagle ping

[sluongng]

Rebase to origin/master + changed my email so that CLA passed

[sluongng]

Switched from using += ["element"] to list.append("elelement") to fix buildifier lint check

[alexeagle]

I think there's still a sharp edge here - if you use stamping, then the image will be different on every build regardless of code changes. So we might want to document it a bit more - but since I just changed the stamping logic to require explicit opt-in (either via stamp = always or --stamp) I'm not as concerned as I would have been previously.

[sluongng]

I think there's still a sharp edge here - if you use stamping, then the image will be different on every build regardless of code changes. So we might want to document it a bit more - but since I just changed the stamping logic to require explicit opt-in (either via stamp = always or --stamp) I'm not as concerned as I would have been previously.

The great thing about OCI format and container registry is that even with stamping, hopefully only the final image of the layer / the json manifest file / the final image tag in the sh_binary push script is affected content wise. In such cases, one should still be comfortable pushing all with minimal storage impacts onto the container registry.

Obviously keeping container images healthy by having re-useable layers will take some effort and not trivial, but it should be do-able if you set it up correctly.

[pcj]

Hi @sluongng, approving this. I think the doc on the attribute is enough documentation, though you probably didn't need to mention the default.