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

feat: implement login/logout UX #413

Merged
merged 4 commits into from
Oct 31, 2022
Merged

feat: implement login/logout UX #413

merged 4 commits into from
Oct 31, 2022

Conversation

JeyJeyGao
Copy link
Contributor

@JeyJeyGao JeyJeyGao commented Oct 25, 2022 •
edited
Loading

implemnt login UI based on https://github.com/notaryproject/notation/blob/main/specs/commandline/login.md fix:

  1. added Login Succeeded message when login succeeded
  2. added username parameter validation
  3. removed --plain-http global flag
  4. updated spec: removed logout --plain-http & --all flag

Test:

  1. login with -u -p flags
  2. login with $NOTATION_USERNAME $NOTATION_PASSWORD
  3. login with --password-stdin
  4. login without username -> username was not set.

Signed-off-by: Junjie Gao [email protected]

@JeyJeyGao JeyJeyGao force-pushed the junjiegao/spec_login branch from e7f9813 to 5027651 Compare October 26, 2022 02:41
@yizha1 yizha1 added this to the beta-1 milestone Oct 26, 2022
chloeyin
chloeyin reviewed Oct 26, 2022
View reviewed changes
cmd/notation/login.go Outdated Show resolved Hide resolved
chloeyin
chloeyin reviewed Oct 26, 2022
View reviewed changes
cmd/notation/logout.go Outdated Show resolved Hide resolved
@yizha1 yizha1 linked an issue Oct 26, 2022 that may be closed by this pull request
Implementing login/logout command #385
Closed
@yizha1 yizha1 requested review from yizha1, priteshbandi, rgnote, shizhMSFT and a team October 26, 2022 06:11
patrickzheng200
patrickzheng200 suggested changes Oct 26, 2022
View reviewed changes
cmd/notation/logout.go Outdated Show resolved Hide resolved
cmd/notation/logout.go Outdated Show resolved Hide resolved
@JeyJeyGao JeyJeyGao force-pushed the junjiegao/spec_login branch from 0ac3223 to f418a78 Compare October 26, 2022 09:09
@yizha1
Copy link
Contributor

yizha1 commented Oct 26, 2022 •
edited
Loading

@dtzar There are some cases need to be considered for --all flag, for example, user will log out from all the registries including those logged in using docker login or other tools. There are also other cases. We need spend more time on investigation. So, I suggest removing --all flag for now considering the time of beta.1 release is approaching and add it back later once we figure out the correct behaviors.

shizhMSFT
shizhMSFT reviewed Oct 27, 2022
View reviewed changes
cmd/notation/login.go Outdated Show resolved Hide resolved
cmd/notation/login.go Outdated Show resolved Hide resolved
@JeyJeyGao JeyJeyGao force-pushed the junjiegao/spec_login branch from 8df0dce to b3a26e1 Compare October 27, 2022 04:27
@yizha1 yizha1 removed this from the beta-1 milestone Oct 27, 2022
shizhMSFT
shizhMSFT approved these changes Oct 27, 2022
View reviewed changes
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

LGTM

@priteshbandi
Copy link
Contributor

priteshbandi commented Oct 28, 2022 •
edited
Loading

@dtzar There are some cases need to be considered for --all flag, for example, user will log out from all the registries including those logged in using docker login or other tools. There are also other cases. We need spend more time on investigation. So, I suggest removing --all flag for now considering the time of beta.1 release is approaching and add it back later once we figure out the correct behaviors.

Should we mention in command help that logout will log out from registries including those logged in using docker login or other tools?

priteshbandi
priteshbandi approved these changes Oct 28, 2022
View reviewed changes
Copy link
Contributor

@priteshbandi priteshbandi left a comment

Choose a reason for hiding this comment

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

LGTM

patrickzheng200
patrickzheng200 approved these changes Oct 28, 2022
View reviewed changes
Copy link
Contributor

@patrickzheng200 patrickzheng200 left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov-commenter
Copy link

codecov-commenter commented Oct 28, 2022 •
edited
Loading

Codecov Report

Merging #413 (325b069) into main (469069e) will not change coverage.
The diff coverage is 85.71%.

@@           Coverage Diff           @@
##             main     #413   +/-   ##
=======================================
  Coverage   33.91%   33.91%           
=======================================
  Files          21       21           
  Lines        1138     1138           
=======================================
  Hits          386      386           
  Misses        744      744           
  Partials        8        8
Impacted Files Coverage Δ
cmd/notation/main.go 0.00% <ø> (ø)
cmd/notation/login.go 42.68% <80.00%> (-0.53%) ⬇️
cmd/notation/logout.go 54.16% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

shizhMSFT
shizhMSFT approved these changes Oct 31, 2022
View reviewed changes
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

LGTM

@JeyJeyGao
feat: implement login/logout UX
20cb77d 
implemnt login UI based on https://github.com/notaryproject/notation/blob/main/specs/commandline/login.md
fix:
1. added `Login Succeeded` message when login succeeded
2. added username parameter validation
3. removed --plain-http global flag

Signed-off-by: Junjie Gao <[email protected]>
@JeyJeyGao
fix: resolve comment
6e84c2f 
Signed-off-by: Junjie Gao <[email protected]>
@JeyJeyGao
fix: resolve comment
c56cdf7 
Signed-off-by: Junjie Gao <[email protected]>
@JeyJeyGao
fix: resolve comments
325b069 
Signed-off-by: Junjie Gao <[email protected]>
@JeyJeyGao JeyJeyGao force-pushed the junjiegao/spec_login branch from 0d8f8c7 to 325b069 Compare October 31, 2022 06:10
shizhMSFT
shizhMSFT approved these changes Oct 31, 2022
View reviewed changes
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

LGTM

@shizhMSFT shizhMSFT merged commit a41b377 into notaryproject:main Oct 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chloeyin chloeyin chloeyin left review comments

@priteshbandi priteshbandi priteshbandi approved these changes

@patrickzheng200 patrickzheng200 patrickzheng200 approved these changes

@shizhMSFT shizhMSFT shizhMSFT approved these changes

@yizha1 yizha1 Awaiting requested review from yizha1 yizha1 is a code owner

@rgnote rgnote Awaiting requested review from rgnote rgnote is a code owner

Assignees

@JeyJeyGao JeyJeyGao

Labels
None yet
Projects
Notary Project Planning Board
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implementing login/logout command
7 participants
@JeyJeyGao @yizha1 @priteshbandi @codecov-commenter @chloeyin @shizhMSFT @patrickzheng200