Skip to content

Add support for repositories (registry namespaced) on login/logout and push/pull #11054

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
Merged
openshift-ci merged 1 commit into from
Aug 1, 2021
Merged

Add support for repositories (registry namespaced) on login/logout and push/pull #11054

openshift-ci merged 1 commit into from
Aug 1, 2021

Conversation

@saschagrunert
Copy link
Member

@saschagrunert saschagrunert commented Jul 27, 2021 •
edited
Loading

This adds the integration tests for the repository or namespaced registry feature introduced in c/common.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 27, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 27, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: saschagrunert

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

The pull request process is described here

Details 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 Jul 27, 2021
@saschagrunert saschagrunert force-pushed the login-logout-path-tests branch 6 times, most recently from 97db102 to f89d214 Compare July 27, 2021 10:27
@saschagrunert saschagrunert changed the title WIP: Add --accept-repositories integration tests Add --accept-repositories integration tests Jul 27, 2021
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 27, 2021
@saschagrunert
Copy link
Member Author

saschagrunert commented Jul 27, 2021

Test are running towards green. Do we need a c/common release or are we fine vendoring the latest master here @vrothberg?

@vrothberg
Copy link
Member

vrothberg commented Jul 27, 2021 •
edited
Loading

Test are running towards green. Do we need a c/common release or are we fine vendoring the latest master here @vrothberg?

I am cool with vendoring commits during development. We just need to make sure to vendor a release before releasing Podman.

vrothberg
vrothberg reviewed Jul 27, 2021
View reviewed changes
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.

besides the doc nit, LGTM

Nice work, @saschagrunert !

@saschagrunert saschagrunert mentioned this pull request Jul 27, 2021
Merged
@saschagrunert saschagrunert force-pushed the login-logout-path-tests branch from f89d214 to 654553b Compare July 27, 2021 13:29
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 27, 2021
@saschagrunert saschagrunert force-pushed the login-logout-path-tests branch from 654553b to ea56c96 Compare July 27, 2021 13:31
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 27, 2021
@saschagrunert
Copy link
Member Author

saschagrunert commented Jul 28, 2021

@containers/podman-maintainers PTAL

@saschagrunert saschagrunert force-pushed the login-logout-path-tests branch from ea56c96 to ba97692 Compare July 28, 2021 07:00
rhatdan
rhatdan reviewed Jul 28, 2021
View reviewed changes
@saschagrunert
Copy link
Member Author

saschagrunert commented Jul 30, 2021

@rhatdan PTAL at the latest discussion points

@saschagrunert saschagrunert self-assigned this Jul 30, 2021
@rhatdan
Copy link
Member

rhatdan commented Jul 30, 2021

I say no option and opt in to new change. IE Breaking change, If we get complaints, then we can add the flag and the ability to manage this in containers/common.

@mtrmac
Copy link
Contributor

mtrmac commented Jul 30, 2021

I say no option and opt in to new change.

OK, that requires a new parameter to c/common/pkg/auth.Get{Login,Logout}Flags. (Or hard-coding the behavior for all callers and dropping the support for the flag entirely? Either way, a c/common change.)

@saschagrunert saschagrunert mentioned this pull request Jul 30, 2021
Merged
@saschagrunert
Copy link
Member Author

saschagrunert commented Jul 30, 2021

Change in c/common: containers/common#700

@saschagrunert saschagrunert force-pushed the login-logout-path-tests branch from ba97692 to 69de9d4 Compare July 30, 2021 11:08
saschagrunert
saschagrunert commented Jul 30, 2021
View reviewed changes
@saschagrunert
Copy link
Member Author

saschagrunert commented Jul 30, 2021

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 30, 2021
@saschagrunert
Add --accept-repositories integration tests
732ece6 
This adds the integration tests for the repository or namespaced
registry feature introduced in c/common.

Signed-off-by: Sascha Grunert <[email protected]>
@saschagrunert saschagrunert force-pushed the login-logout-path-tests branch from 69de9d4 to 732ece6 Compare July 30, 2021 12:54
@saschagrunert
Copy link
Member Author

saschagrunert commented Jul 30, 2021

/hold cancel
This is ready for review and possibly merge

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 30, 2021
@TomSweeneyRedHat
Copy link
Member

TomSweeneyRedHat commented Jul 30, 2021

LGTM

@mtrmac
Copy link
Contributor

mtrmac commented Jul 31, 2021

Any opinions on whether Buildah and Skopeo should opt into the new behavior as well? (For the upcoming releases?)

[Cc: @TomSweeneyRedHat for possible timing interaction, especially vs. a Buildah release before vendoring into Podman — I don’t think it would be a problem to deliver this CLI change in Podman only to minimize last-minute rush, but I might be mistaken about that.]

mtrmac
mtrmac reviewed Jul 31, 2021
View reviewed changes
Copy link
Contributor

@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.

Code LGTM, but I’d prefer someone more knowledgeable to check the timing vs. vendor dance and releases.

@rhatdan
Copy link
Member

rhatdan commented Aug 1, 2021

Yes Buildah and Skopeo should also work the same way.
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 1, 2021
@openshift-ci openshift-ci bot merged commit 4244288 into containers:main Aug 1, 2021
@saschagrunert saschagrunert deleted the login-logout-path-tests branch August 2, 2021 07:09
@saschagrunert saschagrunert changed the title Add --accept-repositories integration tests Add support for repositories (registry namespaced) on login/logout and push/pull Aug 2, 2021
@saschagrunert
Copy link
Member Author

saschagrunert commented Aug 2, 2021 •
edited
Loading

Changed the PR title to reflect the actual change.

This was referenced Aug 2, 2021
Merged
Merged
@saschagrunert
Copy link
Member Author

saschagrunert commented Aug 2, 2021

Yes Buildah and Skopeo should also work the same way.

Created containers/buildah#3412 and containers/skopeo#1396 as follow-up.

@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 Sep 22, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@rhatdan rhatdan rhatdan left review comments

@mtrmac mtrmac mtrmac left review comments

@TomSweeneyRedHat TomSweeneyRedHat TomSweeneyRedHat left review comments

+1 more reviewer

@vrothberg vrothberg vrothberg left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@saschagrunert saschagrunert

@rhatdan rhatdan

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@saschagrunert @vrothberg @rhatdan @mtrmac @TomSweeneyRedHat