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.

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

delete successfully pushed message #1744

Closed
wants to merge 1 commit into from

Conversation

QiWang19
Copy link
Contributor

@QiWang19 QiWang19 commented Jul 28, 2019

close #1742
Delete the line: Successfully pushed...

Signed-off-by: Qi Wang [email protected]

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

Now it's reporting docker://localhost:5000/alpine:latest@sha256:294c4cf3a9a174563142b85737afca6e14d2ef5eaaed9b7cac13cac787c06e25, which is much better but we currently do not support the image:tag@digest notation. So if a user would copy the output and tries to use it, they will face a parsing error. That might not be the best experience.

I see two options (maybe there are more that I don't yet see):

  1. We support parsing image:tag@digest. @mtrmac should weigh in here to be sure I am not missing something.
  2. We remove the entire Printf(...) line (as Podman).

@QiWang19 QiWang19 force-pushed the StringWithinTransport branch from 98c41db to 9ca65be Compare July 29, 2019 13:17
@QiWang19
Copy link
Contributor Author

I remove the Printf(...) for now to keep it same as Podman

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @QiWang19 👍

@TomSweeneyRedHat @nalind @rhatdan PTAL

@TomSweeneyRedHat
Copy link
Member

Change is fine, but the commit title and description need to be touched up to reflect the final changes.

close containers#1742
Delete the line: `Successfully pushed...`

Signed-off-by: Qi Wang <[email protected]>
@QiWang19 QiWang19 force-pushed the StringWithinTransport branch from 9ca65be to ee6be6b Compare July 29, 2019 14:33
@mtrmac
Copy link
Contributor

mtrmac commented Jul 29, 2019

  1. We support parsing image:tag@digest. @mtrmac should weigh in here to be sure I am not missing something.

See earlier discussion in #1407 and linked issues.

@TomSweeneyRedHat TomSweeneyRedHat changed the title fix prefix '//' pushing to registry delete successfully pushed message Jul 29, 2019
@QiWang19
Copy link
Contributor Author

@vrothberg @TomSweeneyRedHat PTAL

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks for taking care of it!

@TomSweeneyRedHat
Copy link
Member

LGTM assuming happy tests

@TomSweeneyRedHat
Copy link
Member

@rh-atomic-bot r+

@rh-atomic-bot
Copy link
Collaborator

📌 Commit ee6be6b has been approved by TomSweeneyRedHat

@rh-atomic-bot
Copy link
Collaborator

⌛ Testing commit ee6be6b with merge a74bdd3...

@rh-atomic-bot
Copy link
Collaborator

☀️ Test successful - status-papr, status-travis
Approved by: TomSweeneyRedHat
Pushing a74bdd3 to master...

@wwilson
Copy link

wwilson commented Aug 15, 2019

Following this change, what's the recommended way to atomically determine the remote digest of a newly-pushed image? We had come to rely on this log message for content addressability, in order to work around the issue that RepoDigest and local image digests do not in general match (containers/skopeo#469).

An alternate workaround would be to skopeo inspect the remote image after push completes, but that's obviously asking for concurrency problems if somebody else uploads an image with the same tag at the same time.

Is there another way that Buildah reports the RemoteDigest of the image it has just pushed? Or should we be going about this a completely different way?

@rhatdan
Copy link
Member

rhatdan commented Aug 16, 2019

@vrothberg @mtrmac @nalind PTAL

TomSweeneyRedHat added a commit to TomSweeneyRedHat/buildah that referenced this pull request Aug 16, 2019
We removed the "Succesfully pushed ..." statement from the end of
the `buildah push` command in containers#1744.  However we've had a request
for that information still so I'm re-adding it as a debug statement
and we can discuss if there's a better way or not.

Signed-off-by: TomSweeneyRedHat <[email protected]>
@TomSweeneyRedHat
Copy link
Member

If you run 'buildah --debug push ...` you get some of that information in the output:

Writing manifest to image destination
DEBU[0001] PUT https://registry-1.docker.io/v2/tomsweeneyredhat/testing/manifests/first 
Storing signatures
DEBU[0002] pushed image "docker.io/tomsweeneyredhat/testing:first@sha256:b552b401d9977f4da64e410727e84a736be5b78f96b561a9b21579b3d1d74a8f" with digest sha256:b552b401d9977f4da64e410727e84a736be5b78f96b561a9b21579b3d1d74a8f 

I'm not sure if that's sufficient for your needs nor do I know of another way to easily do it other than what you've mentioned.

I've just "re-added" the output as a debug line in #1799. If the other folks don't have a better thought, we could go with that. The output using the --debug as shown above would look like:

Writing manifest to image destination
DEBU[0001] PUT https://registry-1.docker.io/v2/tomsweeneyredhat/testing/manifests/first 
Storing signatures
DEBU[0002] pushed image "docker.io/tomsweeneyredhat/testing:first@sha256:b552b401d9977f4da64e410727e84a736be5b78f96b561a9b21579b3d1d74a8f" with digest sha256:b552b401d9977f4da64e410727e84a736be5b78f96b561a9b21579b3d1d74a8f 
DEBU[0002] Successfully pushed //tomsweeneyredhat/testing:first@sha256:b552b401d9977f4da64e410727e84a736be5b78f96b561a9b21579b3d1d74a8f 

There's a lot more output this way though, would this work for you?

@wwilson
Copy link

wwilson commented Aug 16, 2019

Thanks, I didn't know about the buildah --debug push output. I think (after your change) we could skate by with taking that and feeding it into sed/awk, but I'm a little nervous parsing debug output like that since it seems like the kind of thing that can change without warning (as indeed it did in this case).

If you agree that this is a reasonable thing for people to want to know after pushing an image, should we also think about a more permanent and documented solution?

@TomSweeneyRedHat
Copy link
Member

TomSweeneyRedHat commented Aug 16, 2019

I'd like to hear from @nalind and @rhatdan on their thoughts as they had wanted to remove this line. Another approach that we might consider is adding a --verbose option which would print that line as it had previously done if --verbose or -v was passed in ala buildah push -v ...

FWIW, the --debug option can be placed after buildah and before any buildah command to get a lot of insight into what's going on under the covers with a particular command.

@mtrmac
Copy link
Contributor

mtrmac commented Aug 17, 2019

IIRC removing this text was not an explicit goal, it was just easier than to agree on a format that makes sense when we didn’t know of an user and didn’t have a strong need. (Let’s worry about that part in #1799.)

Originally the functionality was added via containers/image#518 for OpenShift use (I guess via the API) and to the CLI output via #1097 , which does not explicitly say why it was added to the CLI).


I mean, if the digest is supposed to be reliably consumable by scripts, we don’t want the scripts scraping the CLI output if we can help it; the CLI could have a buildah push --digestfile (similar to buildah commit --iidfile), or there could be some other structured output format.

--verbose/--debug are easier to implement but ultimately less reliable.

@rhatdan
Copy link
Member

rhatdan commented Aug 17, 2019

Ok, if users want this information, then --digestfile would work. @wwilson WDYT?

@wwilson
Copy link

wwilson commented Aug 17, 2019

I agree that structured output would be best. —digestfile sounds great.

TomSweeneyRedHat added a commit to TomSweeneyRedHat/buildah that referenced this pull request Aug 17, 2019
We removed the "Succesfully pushed ..." statement from the end of
the `buildah push` command in containers#1744.  However we've had a request
for that information still so I'm re-adding it as a debug statement
and we can discuss if there's a better way or not.

Also add the --digestfile option.  With it the user will be able
to specify a file to write the digest of the pushed image to for
reference.

Signed-off-by: TomSweeneyRedHat <[email protected]>
TomSweeneyRedHat added a commit to TomSweeneyRedHat/buildah that referenced this pull request Aug 19, 2019
We removed the "Succesfully pushed ..." statement from the end of
the `buildah push` command in containers#1744.  However we've had a request
for that information still so I'm re-adding it as a debug statement
and we can discuss if there's a better way or not.

Also add the --digestfile option.  With it the user will be able
to specify a file to write the digest of the pushed image to for
reference.

Signed-off-by: TomSweeneyRedHat <[email protected]>
TomSweeneyRedHat added a commit to TomSweeneyRedHat/buildah that referenced this pull request Aug 19, 2019
We removed the "Succesfully pushed ..." statement from the end of
the `buildah push` command in containers#1744.  However we've had a request
for that information still so I'm re-adding it as a debug statement
and we can discuss if there's a better way or not.

Also add the --digestfile option.  With it the user will be able
to specify a file to write the digest of the pushed image to for
reference.

Signed-off-by: TomSweeneyRedHat <[email protected]>
rh-atomic-bot pushed a commit that referenced this pull request Aug 19, 2019
We removed the "Succesfully pushed ..." statement from the end of
the `buildah push` command in #1744.  However we've had a request
for that information still so I'm re-adding it as a debug statement
and we can discuss if there's a better way or not.

Also add the --digestfile option.  With it the user will be able
to specify a file to write the digest of the pushed image to for
reference.

Signed-off-by: TomSweeneyRedHat <[email protected]>

Closes: #1799
Approved by: rhatdan
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pushing to a registry has // prefix
7 participants