-
Notifications
You must be signed in to change notification settings - Fork 84
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
feat: Support username and password prompt in login #566
Conversation
I noticed the current login tests only cover preRun(). Is there any reason of doing this? Do we want to also cover RunE()? My change is in RunE() so it won't be covered in the test file following current tests. |
@JeyJeyGao Could you pls help to confirm and provide your suggestion? |
Can we create new tests for this usecase? |
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## main #566 +/- ##
==========================================
- Coverage 34.36% 33.68% -0.68%
==========================================
Files 32 32
Lines 1848 1885 +37
==========================================
Hits 635 635
- Misses 1192 1229 +37
Partials 21 21
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
We may need to add an E2E test for it because username and password are read from STDIN. The E2E test is here. |
I noticed the unit tests only produce the commands instead of running the commands. E2e tests run the commands. So I added the tests to e2e tests. |
8ba8835
to
332d585
Compare
@JeyJeyGao I completed most of the e2e tests but the only thing left is credential helper. I noticed it seems notation forces users to have credential helper. If not, it will directly throw error
This is different with other container tools. For example, in nerdctl, it throws I'm wondering is it intentionally to enforce notation users to have credential helper? |
Yes, it depends on credential helper now, but in the next few months, login without credential helper will be supported. Currently, a If the Github action machine doesn't have a credential helper, we cannot do
|
@JeyJeyGao Great! Could you point me the link of credential-go? Login without credential helper is pretty valuable to my use case. Just want to track the progress and see if there is anything I can help. |
|
@JeyJeyGao Pushed the changes except the credential helper in Github action. I set up credential helper in my host and current e2e tests work find. In my investigation, to set up credential helper, there are 3 steps
Let me know if you have any advice to deal with gpg2. Otherwise we may temporarily skip the automated e2e tests or wait for credential-go, but @FeynmanZhou may want to have this sooner than later. |
@JeyJeyGao I added Skip() to the new e2e tests to unblock Github actions, and created an issue to track with puting it in the comment. #587 |
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.
LGTM
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.
LGTM with nit comment.
Signed-off-by: Ziwen Ning <ningziwe@amazon.com>
31ff222
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.
LGTM
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.
LGTM with suggestions.
#503
Added e2e tests. The current unit tests don't have RunE() testing code. Adding the unit test could lead to lots of refactoring. I'm happy to help the refactoring and start the unit tests of RunE() but I think it is better to be in a separated PR. Adding lots of unit test refactoring could mess up this PR.
TODO: Seems login requires credential helper. Need to install credential helper as a test step.