-
Notifications
You must be signed in to change notification settings - Fork 86
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
Add support for custom keyring names on linux #47
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: JojiiOfficial <jojii@jojii.de>
Thanks for the pr, 2 comments from my side. |
func (s secretServiceProvider) Set(service, user, password string) error { return nil } | ||
|
||
// Get password from keyring given service and user name. | ||
func (s secretServiceProvider) Get(service, user string) (string, error) { 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.
Should not this return DefaultKeyringName?
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.
Those functions aren't getting called at all. Those are just for the compiler to not throw an error. Otherwise on any other GOOS than linux the compiler would say that there is no such secretServiceProvider to typecast to and it wouldn't compile.
Same in keyring_darwin.go
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.
The DefaultKeyring is picked here:
https://github.com/JojiiOfficial/go-keyring/blob/master/secret_service/secret_service.go#L110
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.
For me it looks like you should refactor this part, such that the empty string check should be dropped and the default should be returned by the new functions created.
@mikkeloscar Wdyt?
func (s secretServiceProvider) Set(service, user, password string) error { return nil } | ||
|
||
// Get password from keyring given service and user name. | ||
func (s secretServiceProvider) Get(service, user string) (string, error) { 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.
Should not this return DefaultKeyringName?
} | ||
|
||
// NewSecretService inializes a new SecretService object. | ||
func NewSecretService() (*SecretService, error) { | ||
func NewSecretService(keyringName string) (*SecretService, error) { |
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 breaking change in libraries is not a great thing to do.
Please let the signature like this and add another func:
func WithKeyringName(name string) (*SecretService, error) {
s,err := NewSecretService()
if err != nil { return nil, err }
s.KeyringName = name
return s, 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.
There are a couple of comments that were not addressed and I just added one other.
I've implemented a way to use custom keyring names (on linux).
To let it compile on another OS than linux, every
keyring_<os>.go
file needs to implement thesecretServiceProvider
to allow a typecast in thekeyring.go
. This is required since there must be a way to set the keyringName in thekeyring_linux.go
fromkeyring.go
to pass the desired keyring name.I've successfully cross compiled a test application for every os
A test function for each function from the keyring interface is available as well.
To use it a
customKeyring
object must be created usingkeyring.NewCustomKeyring("name")
.Fixes #46