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

[DO NOT MERGE] Add push with only digest support #20486

Closed
wants to merge 1 commit into from
Closed

[DO NOT MERGE] Add push with only digest support #20486

wants to merge 1 commit into from

Conversation

umohnani8
Copy link
Member

[NO NEW TESTS NEEDED]

Does this PR introduce a user-facing change?

None

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 25, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: umohnani8

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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 25, 2023
@packit-as-a-service
Copy link

Cockpit tests failed for commit ce0de483e91a5cbe46533add31bd96c44de27ecd. @martinpitt, @jelly, @mvollmer please check.

@umohnani8 umohnani8 force-pushed the untagged branch 2 times, most recently from 79cf834 to 6f5058a Compare November 14, 2023 16:41
@packit-as-a-service Packit-as-a-Service
Copy link

Cockpit tests failed for commit 6f5058ab1a534447aaa27cfe9967a00f24fc5bf0. @martinpitt, @jelly, @mvollmer please check.

@packit-as-a-service Packit-as-a-Service
Copy link

Cockpit tests failed for commit 79cf834f2507de3e8e423ab624207f9e4049b1e2. @martinpitt, @jelly, @mvollmer please check.

@packit-as-a-service Packit-as-a-Service
Copy link

Cockpit tests failed for commit 6f5058ab1a534447aaa27cfe9967a00f24fc5bf0. @martinpitt, @jelly, @mvollmer please check.

@packit-as-a-service Packit-as-a-Service
Copy link

Cockpit tests failed for commit 79cf834f2507de3e8e423ab624207f9e4049b1e2. @martinpitt, @jelly, @mvollmer please check.

mtrmac
mtrmac reviewed Nov 14, 2023
View reviewed changes
Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

This

  • changes the semantics of previously-valid tag destinations. I think this needs to be a new syntax.
  • with editing docker/reference, also conceptually suggests that references used in other contexts (docker-daemon:, containers-storage, Podman remote API) can be “digest only”; this feature only targets c/image/docker, so it seems to me that it should are confined there.

vendor/github.com/containers/image/v5/docker/docker_transport.go Outdated Show resolved Hide resolved
vendor/github.com/containers/image/v5/docker/docker_transport.go Outdated Show resolved Hide resolved
@umohnani8
Copy link
Member Author

@mtrmac I pushed some updates based on our discussions, PTAL

mtrmac
mtrmac reviewed Nov 21, 2023
View reviewed changes
Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

For earlier context, #20486 (comment) .

cmd/podman/images/push.go Outdated Show resolved Hide resolved
vendor/github.com/containers/image/v5/docker/docker_image_src.go Outdated Show resolved Hide resolved
vendor/github.com/containers/image/v5/docker/docker_transport.go Outdated Show resolved Hide resolved
vendor/github.com/containers/image/v5/docker/docker_transport.go Outdated Show resolved Hide resolved
vendor/github.com/containers/image/v5/docker/docker_transport.go Outdated Show resolved Hide resolved
vendor/github.com/containers/image/v5/docker/reference/normalize.go Outdated Show resolved Hide resolved
vendor/github.com/containers/image/v5/docker/docker_transport.go Show resolved Hide resolved
@umohnani8
Add push with only digest support
c7e6803 
[NO NEW TESTS NEEDED]

Signed-off-by: Urvashi Mohnani <[email protected]>
mtrmac
mtrmac reviewed Nov 29, 2023
View reviewed changes
vendor/github.com/containers/image/v5/docker/docker_image_src.go Show resolved Hide resolved
vendor/github.com/containers/image/v5/docker/docker_transport.go Show resolved Hide resolved
vendor/github.com/containers/image/v5/docker/docker_transport.go
func newReference(ref reference.Named) (dockerReference, error) {
if reference.IsNameOnly(ref) {
func newReference(ref reference.Named, unknownDigest bool) (dockerReference, error) {
if (reference.IsNameOnly(ref) && !unknownDigest) || (!reference.IsNameOnly(ref) && unknownDigest) {
return dockerReference{}, fmt.Errorf("Docker reference %s has neither a tag nor a digest", reference.FamiliarString(ref))
Copy link
Collaborator

Choose a reason for hiding this comment

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

The error message is now misleading for the new case.

vendor/github.com/containers/image/v5/docker/docker_transport.go Show resolved Hide resolved
vendor/github.com/containers/image/v5/docker/docker_transport.go Show resolved Hide resolved
vendor/github.com/containers/image/v5/docker/docker_transport.go Show resolved Hide resolved
vendor/github.com/containers/image/v5/docker/docker_transport.go Show resolved Hide resolved
vendor/github.com/containers/image/v5/docker/docker_image_dest.go Show resolved Hide resolved
@umohnani8 umohnani8 mentioned this pull request Dec 4, 2023
Merged
@umohnani8
Copy link
Member Author

umohnani8 commented Dec 11, 2023 •
edited
Loading

PR merged in c/image - will open a PR with vendor and farm build changes

@umohnani8 umohnani8 closed this Dec 11, 2023
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Mar 12, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mtrmac mtrmac mtrmac left review comments

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@umohnani8 @mtrmac