Skip to content

Pick registry to login from full image name as well #1905

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-merge-robot merged 1 commit into from
Dec 6, 2018
Merged

Pick registry to login from full image name as well #1905

openshift-merge-robot merged 1 commit into from
Dec 6, 2018

Conversation

@umohnani8
Copy link
Member

@umohnani8 umohnani8 commented Nov 30, 2018

podman login reg.io/username/image works as well now. It picks
the registry and checks for authentication, if none exist it
will prompt for username and password.

Fixes #1690
Waiting for containers/image#535 to get in. Will fix up the vendor.conf and remove the WIP once that is done.

Signed-off-by: Urvashi Mohnani [email protected]

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 30, 2018
@rh-atomic-bot
Copy link
Collaborator

rh-atomic-bot commented Nov 30, 2018

☔ The latest upstream changes (presumably #1317) made this pull request unmergeable. Please resolve the merge conflicts.

mtrmac
mtrmac reviewed Dec 1, 2018
View reviewed changes
@mtrmac mtrmac mentioned this pull request Dec 1, 2018
Closed
@openshift-bot
Copy link

openshift-bot commented Dec 2, 2018

@umohnani8: PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

1 similar comment
@openshift-bot
Copy link

openshift-bot commented Dec 3, 2018

@umohnani8: PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 3, 2018
@umohnani8 umohnani8 force-pushed the login branch 2 times, most recently from 270361b to 8fe2609 Compare December 4, 2018 17:27
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 4, 2018
@umohnani8 umohnani8 changed the title [WIP] Pick registry to login from full image name as well Pick registry to login from full image name as well Dec 4, 2018
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 4, 2018
@umohnani8
Copy link
Member Author

umohnani8 commented Dec 4, 2018

Should be ready to merge once tests are green.

mtrmac
mtrmac reviewed Dec 4, 2018
View reviewed changes
@mheon mheon mentioned this pull request Dec 4, 2018
Closed
@baude
Copy link
Member

baude commented Dec 6, 2018

rebase to fix this

@umohnani8
Pick registry to login from full image name as well
a75b397 
podman login reg.io/username/image works as well now. It picks
the registry and checks for authentication, if none exist it
will prompt for username and password.
If the credentials exist but are not valid, it will prompt the
user for new valid credentials.

Signed-off-by: Urvashi Mohnani <[email protected]>
@umohnani8
Copy link
Member Author

umohnani8 commented Dec 6, 2018

Rebased

@umohnani8
Copy link
Member Author

umohnani8 commented Dec 6, 2018

Should be ready to merge. @rhatdan @mheon @baude PTAL

@TomSweeneyRedHat
Copy link
Member

TomSweeneyRedHat commented Dec 6, 2018

LGTM

@baude
Copy link
Member

baude commented Dec 6, 2018

/lgtm

@baude
Copy link
Member

baude commented Dec 6, 2018

/approve

mtrmac
mtrmac reviewed Dec 6, 2018
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.

LGTM.

cmd/podman/login.go
var (
username string
password string
)
Copy link
Contributor

@mtrmac mtrmac Dec 6, 2018

Choose a reason for hiding this comment

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

(Absolutely non-blocking nit: These declarations are unnecessary. The first CheckAuth can use *FromAuthFile directly, and the getUserAndPass can use := for the two variables.)

cmd/podman/login.go

// registryFromFullName gets the registry from the input. If the input is of the form
// quay.io/myuser/myimage, it will parse it and just return quay.io
// It also returns true if a full image name was given
Copy link
Contributor

@mtrmac mtrmac Dec 6, 2018

Choose a reason for hiding this comment

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

(Non-blocking nit: This function (no longer?) returns a second boolean value.)

mtrmac
mtrmac reviewed Dec 6, 2018
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.

LGTM.

mtrmac
mtrmac reviewed Dec 6, 2018
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.

LGTM

@rhatdan
Copy link
Member

rhatdan commented Dec 6, 2018

/approve
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 6, 2018
@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Dec 6, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: baude, rhatdan

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-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 6, 2018
@openshift-merge-robot openshift-merge-robot merged commit fb3ceeb into containers:master Dec 6, 2018
This was referenced Dec 6, 2018
Merged
Merged
@umohnani8 umohnani8 deleted the login branch October 27, 2020 14:27
@mtrmac mtrmac mentioned this pull request Jul 2, 2021
Merged
@mtrmac mtrmac mentioned this pull request Jul 29, 2021
Merged
@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 24, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@mtrmac mtrmac mtrmac left review comments

@baude baude Awaiting requested review from baude

@mrunalp mrunalp Awaiting requested review from mrunalp

Assignees

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

the issue about podman login

9 participants

@umohnani8 @rh-atomic-bot @openshift-bot @baude @TomSweeneyRedHat @rhatdan @openshift-ci-robot @mtrmac @openshift-merge-robot