-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Decouple NewKeyringFromDir() from sdk.GetConfig() #5547
Conversation
NewKeyringFromDir() takes only a keyring service name, which becomes a component of the directory used as keyring's storage.
No changelog entry is required as the kerying hasn't been released yet |
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.
utACK
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.
Reviewing this PR has me thinking that the constructors as they are now may not make the most sense, namely, NewKeyring*
, which internally makes some use of viper
.
I would propose we instead have a single constructor:
NewKeyring(svcName, backend, rootDir string, input io.Reader, opts ...keys.KeybaseOption)
Force the upstream caller to provide svcName, backend, rootDir
. The caller may then use viper
and sdk.GetConfig()
as they wish. Thoughts?
I like it @alexanderbez! Do you mind if I do this change in an ensuing PR? |
I think thats a great idea @alexanderbez and one that we should follow through on. This PR seems a good venue to deal with that. |
Mhhh, may I ask why? It would undo the work in this PR essentially. In addition, the changes would be pretty minimal and trivial. |
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.
ACK 🎉
Codecov Report
@@ Coverage Diff @@
## master #5547 +/- ##
==========================================
+ Coverage 54.15% 54.16% +0.01%
==========================================
Files 313 313
Lines 18960 18960
==========================================
+ Hits 10268 10270 +2
+ Misses 7894 7892 -2
Partials 798 798
|
NewKeyringFromDir() takes only a keyring service name, which
becomes a component of the directory used as keyring's storage.
For contributor use:
docs/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerFor admin use:
WIP
,R4R
,docs
, etc)