Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Show digest of pushed images #1737

Merged
merged 3 commits into from
Jan 24, 2022
Merged

Show digest of pushed images #1737

merged 3 commits into from
Jan 24, 2022

Conversation

illicitonion
Copy link
Contributor

No description provided.

@google-cla google-cla bot added the cla: yes label Feb 18, 2021
@zoidyzoidzoid
Copy link
Contributor

This looks good to me.

Rebasing may bump some things to reassign who should approve this, since the owners changed in a4d580e

It'd be great to have tests for some of these binaries, but that's definitely out of scope of this PR.

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: illicitonion
To complete the pull request process, please assign after the PR has been reviewed.
You can assign the PR to them by writing /assign 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:

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

alexeagle
alexeagle reviewed Mar 12, 2021
View reviewed changes
Copy link
Contributor

@alexeagle alexeagle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @illicitonion !!

I'm curious what's the motivation for this change? Did you want to copy the digest to paste somewhere? compare the digest with some other reference?

container/go/cmd/pusher/pusher.go Outdated
@@ -121,11 +121,16 @@ func main() {
log.Printf("Destination %s was resolved to %s after stamping.", *dst, stamped)
}

digest, err := img.Digest()
if err != nil {
log.Fatalf("Failed to digest image: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: in the interest of preserving compatibility, could you make this non-fatal? I don't think there's a case where the digest fails but if it did we can just print the original message

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely - done

@illicitonion
Copy link
Contributor Author

Hi @illicitonion !!

👋 🎉

I'm curious what's the motivation for this change? Did you want to copy the digest to paste somewhere? compare the digest with some other reference?

Yeah, we have deployment manifests which list the sha256 of the docker images that should be deployed, and so with this change we can trivially copy&paste / scrape them from logs to put them in our deployment manifests. Without this change, we need to look externally for information from the container registry, which it's nice to avoid needing to do, given we already know the information here :)

@alexeagle
Copy link
Contributor

Ah, I think you're just looking for the .digest-suffixed output of container_push which gives you exactly that info in a file. Then you don't have to scrape the message either - does that work for you?
https://github.com/bazelbuild/rules_docker/blob/master/container/push.bzl#L217-L219

@alexeagle alexeagle added the Can Close? Will close in 30 days unless there is a comment indicating why not label Mar 12, 2021
@illicitonion
Copy link
Contributor Author

Ah, I think you're just looking for the .digest-suffixed output of container_push which gives you exactly that info in a file. Then you don't have to scrape the message either - does that work for you?
https://github.com/bazelbuild/rules_docker/blob/master/container/push.bzl#L217-L219

I'm currently using https://github.com/bazelbuild/rules_docker/blob/master/contrib/push-all.bzl which doesn't expose this at the moment. I'm currently just running bazel run //some:push-all, and it's not obvious how we'd wire it up to proxy those things through... Any ideas on how we'd go about that?

I guess we'd need to go from one bazel invocation to two, so we could drive building the digest files before/after pushing them?

@github-actions github-actions bot removed the Can Close? Will close in 30 days unless there is a comment indicating why not label Mar 13, 2021
@github-actions
Copy link

This Pull Request has been automatically marked as stale because it has not had any activity for 180 days. It will be closed if no further activity occurs in 30 days.
Collaborators can add an assignee to keep this open indefinitely. Thanks for your contributions to rules_docker!

@github-actions github-actions bot added the Can Close? Will close in 30 days unless there is a comment indicating why not label Sep 10, 2021
@github-actions
Copy link

This PR was automatically closed because it went 30 days without a reply since it was labeled "Can Close?"

@github-actions github-actions bot closed this Oct 10, 2021
@alexeagle alexeagle self-assigned this Oct 11, 2021
@alexeagle alexeagle reopened this Oct 11, 2021
@pcj
Copy link
Member

pcj commented Jan 21, 2022

I've also had to resort to extra steps to retrieve the digest myself, so this is a welcome change.

It would be useful IMHO if the printed string was something like Successfully pushed image to us.gcr.io/foo/ubuntu@sha256:digest, because then it can be immediately copy-pasted into a kubernetes yaml or docker pull command.

pcj
pcj approved these changes Jan 21, 2022
View reviewed changes
Copy link
Member

@pcj pcj left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving, but would welcome a slightly different format (see comment above)

@illicitonion
Copy link
Contributor Author

Edited to be easier to copy and paste - I didn't want to remove the old text because the label substitution may also be relevant, so I had it do both if the stamped label isn't a digest.

@alexeagle alexeagle merged commit c27194b into bazelbuild:master Jan 24, 2022
@illicitonion illicitonion deleted the digest branch January 24, 2022 14:48
@gwuah
Copy link

gwuah commented Feb 27, 2023

This is a small change that has made my work/life very easy. thanks for this PR @illicitonion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexeagle alexeagle alexeagle left review comments

@pcj pcj pcj approved these changes

@DaveGay DaveGay Awaiting requested review from DaveGay

@eytankidron eytankidron Awaiting requested review from eytankidron

@smukherj1 smukherj1 Awaiting requested review from smukherj1 smukherj1 is a code owner

@hicksjoseph hicksjoseph Awaiting requested review from hicksjoseph

@gravypod gravypod Awaiting requested review from gravypod gravypod is a code owner

Assignees

@alexeagle alexeagle

Labels
Can Close? Will close in 30 days unless there is a comment indicating why not cla: yes size/XS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@illicitonion @zoidyzoidzoid @k8s-ci-robot @alexeagle @pcj @gwuah