-
Notifications
You must be signed in to change notification settings - Fork 0
Replace zalando/go-keyring with 99designs/keyring #19
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
base: carole/drg-450
Are you sure you want to change the base?
Conversation
d5e4223 to
24d9d64
Compare
24d9d64 to
fc3a6e3
Compare
silv-io
left a comment
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.
Looks good to me in general, just some minor comments.
Nice that we can use something with integrated file fallback 🚀
|
|
||
| config := keyring.Config{ | ||
| ServiceName: keyringService, | ||
| FileDir: filepath.Join(configDir, "localstack"), |
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.
can use configDir from internal/config.go
(guess will need to rename to ConfigDir)
| configDir, _ := os.UserConfigDir() | ||
| config := keyring.Config{ | ||
| ServiceName: "localstack", | ||
| FileDir: filepath.Join(configDir, "localstack"), |
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.
same here with the config path
|
|
||
| func keyringGet(service, user string) (string, error) { | ||
| key := fmt.Sprintf("%s/%s", service, user) | ||
| item, err := testKeyring.Get(key) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| return string(item.Data), nil | ||
| } | ||
|
|
||
| func keyringSet(service, user, password string) error { | ||
| key := fmt.Sprintf("%s/%s", service, user) | ||
| return testKeyring.Set(keyring.Item{ | ||
| Key: key, | ||
| Data: []byte(password), | ||
| }) | ||
| } | ||
|
|
||
| func keyringDelete(service, user string) error { | ||
| key := fmt.Sprintf("%s/%s", service, user) | ||
| err := testKeyring.Remove(key) | ||
| if err == keyring.ErrKeyNotFound || os.IsNotExist(err) { | ||
| return nil | ||
| } | ||
| return 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.
nit: with these it was a bit confusing to me in the other locations why it's suddenly keyringGet vs keyring.Get. Maybe we can call it testKeyringGet etc.?
Switches to
99designs/keyringfor automatic fallback to encrypted file storage when system keyring is unavailable.The previous implementation using
zalando/go-keyringwould fail without a keyring service. This would have required a custom fallback implementation (more code to maintain).99designs/keyringhas low maintenance but is stable (keyring APIs rarely change).Fallback Strategy
Storage Locations
Using
os.UserConfigDir()for OS-dependant paths:~/.config/localstack/~/Library/Application Support/localstack/%APPDATA%\localstack\