-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Unload keys from agent upon logout. #1761
Conversation
38585fe
to
88361e4
Compare
} | ||
|
||
return nil | ||
} |
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.
wonder why you decided to expand the one-liner this way
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.
Whenever I am reading code I always miss the one-liners since they don't follow the typical Go pattern calling a function then checking the return value.
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.
couple of comments
func (tc *TeleportClient) Logout() error { | ||
return trace.Wrap(tc.localAgent.DeleteKey()) | ||
err := tc.localAgent.DeleteKey() |
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.
trace.Wrap(nil)
returns nil
so this change is not necessary
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.
This makes code substantially more readable. I can't count then number of times I've skipped over these one liners when reading code only to have to come back to them.
lib/client/api.go
Outdated
return nil | ||
} | ||
|
||
// LogoutAll removes all certificate for all users from the filesystem |
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.
all certificates
cf.Proxy = "dummy:1234" | ||
tc, err := makeClient(cf, true) | ||
if err != nil { | ||
utils.FatalError(err) |
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 still prefer a return here.
tool/tsh/tsh.go
Outdated
} | ||
|
||
// Remove all keys from disk and the running agent. | ||
err = tc.LogoutAll() | ||
if err != nil { | ||
fmt.Printf("Unable to remove all keys: %v\n", err) |
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.
if we failed here, why do we exit with 0 error code then?
88361e4
to
5eddc32
Compare
can this be merged? |
@kontsevoy yep |
Purpose
Unload keys from agent upon logout.
Implementation
Related Issues
Fixes #1541