-
Notifications
You must be signed in to change notification settings - Fork 611
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
enhance login prompt #752
enhance login prompt #752
Conversation
7890f57
to
d3d332c
Compare
Not entering the username raises the following error I had the same issue when assigning the username, By not entering username, the value will be a newline and option.username raises the above error. |
Please squash |
cmd/nerdctl/login_unix.go
Outdated
if term.IsTerminal(syscall.Stdin) { | ||
fd = os.Stdin | ||
} else { | ||
tty, err := os.Open("/dev/tty") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn’t we just read stdin even when it is not a terminal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to know the use case that one would pass username and password by stdin and why, then password alone has such case, because of secret that is passed but username as stdin? docker login has only password stdin option.
cat secret-password | docker login --username foo --password-stdin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding username to stdin is not intuitive without username-stdin
which is rarely used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, readUsername()
should return an error when stdin is not a terminal?
I don't think we have to open /dev/tty
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a follow-up commit to avoid /dev/tty
804d2ce
to
ca2fec4
Compare
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
ca2fec4
to
c933f8e
Compare
adb7d1f
to
5f6fe17
Compare
Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
5f6fe17
to
5a999f4
Compare
replace #641 ( thanks @simorgh1 )
Signed-off-by: fahed dorgaa fahed.dorgaa@gmail.com