Conversation
Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
Benehiko
force-pushed
the
keychain-interfaces
branch
from
0652beb to
19b8ac7
Compare
|
|
||
| // Erase implements secrets.Store. | ||
| func (k *keychainStore[T]) Delete(ctx context.Context, id store.ID) error { | ||
| panic("unimplemented") |
There was a problem hiding this comment.
I don't get the point of these. Is it just documentation purpose? Or do you tend to implement them later?
I don't see much value in the code as is, but maybe I'm missing something.
There was a problem hiding this comment.
it's to satisfy the store.Store interface. The actual implementation has been done somewhere else. I just wanted to break up some of the PR.
| ) | ||
|
|
||
| type MockStore struct { | ||
| lock sync.RWMutex |
There was a problem hiding this comment.
nit: m (at least in piñata we always name this m)
There was a problem hiding this comment.
I'm not so sure I understand why m is better than lock, sounds too obscure.
There was a problem hiding this comment.
searching through containerd/containerd I see a lot of lock being used. Other variations are also used: mu, m, l and rwlock.
| store map[store.ID]store.Secret | ||
| } | ||
|
|
||
| func (m *MockStore) init() { |
There was a problem hiding this comment.
nit: s (if we'd go with renaming lock to m)
| store map[store.ID]store.Secret | ||
| } | ||
|
|
||
| func (m *MockStore) init() { |
There was a problem hiding this comment.
Why not have a new function instead?
There was a problem hiding this comment.
right, I meant newSomething, eg newMockStore
There was a problem hiding this comment.
I can rename it to tryInit instead. newSomething sounds like it will return a new object
Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
joe0BAB
left a comment
joe0BAB
left a comment
There was a problem hiding this comment.
LGTM! (I'd still prefer not having the init function on the mock, but it's not blocking to me)
2 participants
Required by #16
This patch adds the
keychainpackage inside thestoremodule, but does not implement an actual underlying keychain provider.