client: unexport the watcher type and update NewUpdater #118
Conversation
creachadair
force-pushed
the
mjf/simplifex
branch
from
be6e9e2
to
62f124b
Compare
creachadair
force-pushed
the
mjf/simplifex
branch
from
62f124b
to
e9333a6
Compare
creachadair
force-pushed
the
mjf/simplifex
branch
from
e9333a6
to
09ffa37
Compare
There was a problem hiding this comment.
Does each intermediate commit build, with this order of unexporting before changing NewUpdater's signature? Obviously NewUpdater becomes unusable in one of the intermediate commits until a later one, but as long as each commit builds that seems okay.
(I'm trying to work out whether you can rebase-and-merge or whether you should squash-and-merge, to try and keep all commits in the history buildable where possible)
Either way, LGTM!
This comment was marked as resolved.
This comment was marked as resolved.
creachadair
force-pushed
the
mjf/simplifex
branch
2 times, most recently
from
f717e7b
to
60ca42f
Compare
This comment was marked as resolved.
This comment was marked as resolved.
creachadair
force-pushed
the
mjf/simplifex
branch
from
60ca42f
to
24b6f2e
Compare
The watcher semantics are still useful for plumbing, but it's hard to use correctly, so unexport it. Now that watcher is unexported, move its tests from a separate package into an internal package test. It is now no longer necessary to have the "watcher" method, so remove it. An Updater now no longer accepts a watcher (which is unexported), but instead takes a *Store and the desired secret name directly. Rework the constructor to allow reporting an error. Add a StaticUpdater constructor to make an Updater that vends a static value, analogous to StaticSecret.
creachadair
force-pushed
the
mjf/simplifex
branch
from
24b6f2e
to
af2bea6
Compare
creachadair
changed the title
There was a problem hiding this comment.
This test is moved from store_test.go, so that it can test the now-unexported type.
| @@ -291,71 +287,6 @@ func TestSlowInit(t *testing.T) { | |||
| checkSecretValue(t, st, "boo", "go for the eyes") | |||
| } | |||
|
|
|||
| func TestWatcher(t *testing.T) { | |||
There was a problem hiding this comment.
Moved to internal_test.go
There was a problem hiding this comment.
Moved from store.go, and unexported.
|
@danderson This is (finally) ready for another look, I had to go make sure I could port the existing use cases and in doing so discovered more was needed. |
| // The value reported by the updater never changes. | ||
| func StaticUpdater[T any](fixedValue T) *Updater[T] { | ||
| return &Updater[T]{ | ||
| newValue: panicOnUpdate[T], |
There was a problem hiding this comment.
To be sure I understand: barring a bug in this library, this should never get called, correct? In particular, there's no way for a user of this library to hold things wrong enough to trigger the panic?
There was a problem hiding this comment.
That is correct—the only way that could happen is if a new secret value were delivered, which cannot happen as there is no Store. This is mainly here as documentation.
I neglected to include this in #118
I neglected to include this in #118
I neglected to include this in #118
I neglected to include this in #118
I neglected to do this in #118.
I neglected to do this in #118.
The watcher semantics are still useful for plumbing, but it's hard to use
correctly, so unexport it. Now that watcher is unexported, move its tests from
a separate package into an internal package test.
It is now no longer necessary to have the "watcher" method, so remove it.
An Updater now no longer accepts a watcher (which is unexported), but instead
takes a *Store and the desired secret name directly. Rework the constructor to
allow reporting an error.
Add a StaticUpdater constructor to make an Updater that vends a static value,
analogous to StaticSecret.