-
Notifications
You must be signed in to change notification settings - Fork 9
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
Upgrade lasso and use incognito mode. #187
Conversation
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.
First look!
Co-authored-by: Shalini Khare <[email protected]>
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.
Design question.
e284e08
to
913f69b
Compare
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.
@goagain: Overall, I think this is looking great! Very clean code here. However, I think we should add some tests.
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.
@goagain I beleive the core business logic doesn't warrent unit tests, because it's mostly just a new CLI command, and printing some info. Managing the Machine ID itself, is worth unit testing, and that is owned by Lasso.
After seeing that interface in use here, I think it would benfit us to make an update in Lasso, to handle fully unit testing the "MachineIDHelper" class. That class can have a more complete interface that handles all this.
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 have 1 last design question :)
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.
A few final suggestions on formatting and wording.
Co-authored-by: Kyle W. Rader <[email protected]>
To achieve privacy compliance, AzureAuth need to stop collecting EUII data, such as alias, device name.
This PR will use the functionality that Lasso provides.
To make sure we are still able to support our customer, subcommand
info
is added.Run
azureauth info
will get the following output:Also, the helper text is added in help manual
azureauth --help
: