feat: added logout command #379
Conversation
Signed-off-by: Markus Sommer <markus.sommer@postfinance.ch>
main.go
Outdated
| go func() { | ||
| <-sigs | ||
| cancel() | ||
| }() |
There was a problem hiding this comment.
i copied it from the login command above. however i think we coud simply all the smae into
ctx, cancel := signal.NotifyContext(context.Background(), syscall.SIGINT, syscall.SIGTERM) defer cancel()
There was a problem hiding this comment.
login needs this because it does some complex stuff with channels that causes the SIGTERM to hang unless we do this. Maybe we need it hear, maybe we don't, I wanted to know if we needed it because I wouldn't expect the sigterm call to hang for this functionality and I'd try to find the source of it.
commands/logout.go
Outdated
| afs := &afero.Afero{Fs: l.Fs} | ||
|
|
||
| // empty config file | ||
| err = afs.WriteFile(userOpkSshConfig, []byte{}, 0o600) |
There was a problem hiding this comment.
Why is logout creating a config file? Shouldn't it only be deleting keys?
There was a problem hiding this comment.
the login --configure adds an include directive to the users .ssh/config file includeing the given file. my intention was that referceing a non existing file in my config is not good. i will test it, if ssh complains. if not, i can remove the file instead.
also cleaning up the user config is not good i think, because the user needs to do the configure again.
There was a problem hiding this comment.
I agree we shouldn't be deleting the config, but if no config exists, I don't think we should be creating it.
commands/logout.go
Outdated
| return fmt.Errorf("key type (%s) has no default output file name; use -i <filePath>", l.KeyTypeArg.String()) | ||
| } | ||
|
|
||
| for _, keyFilename := range keyFileNames { |
There was a problem hiding this comment.
Can you make this code more clear that we checking if a key is an opkssh before removing it. I initially thought the function removeKeys, based on the name, just deleted the keys without performing any check.
|
@marsom Thanks for this PR. Very glad to see someone working on this issue. Can you provide a more detailed PR description and also tests? Since you imported the afero.Fs pattern, unittests shouldn't be that difficult to write |
I will update the branch |
Signed-off-by: Markus Sommer <markus.sommer@postfinance.ch>
2 participants
like discussed in #311