Add a config source manager #2857
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2857 +/- ##
==========================================
- Coverage 91.74% 91.66% -0.08%
==========================================
Files 286 287 +1
Lines 15086 15221 +135
==========================================
+ Hits 13841 13953 +112
- Misses 851 865 +14
- Partials 394 403 +9
Continue to review full report at Codecov.
|
| m.sessions[cfgSrcName] = session | ||
| } | ||
|
|
||
| retrieved, err := session.Retrieve(ctx, selector, params) |
There was a problem hiding this comment.
Is this a blocking call? So we will not indicate in any way that there is an ongoing retrieval from a remote source, which potentially can take a lot of time. If so perhaps in the future we should add some sort of logging or something to show this.
There was a problem hiding this comment.
Yes, this is potentially a blocking call as is the NewSession. I will add a parameter to create the manager to include the logger as we typically flow it to components.
| return nil, fmt.Errorf("config source %q not found", cfgSrcName) | ||
| } | ||
|
|
||
| session, err = cfgSrc.NewSession(ctx) |
There was a problem hiding this comment.
For ConfigSource.NewSession docs say:
// The code managing the returned Session object must guarantee that the object is not used
// concurrently and that a single ConfigSource only have one Session open at any time.
Are we violating the second requirement here? If the same cfgSourceName is referenced from multiple places then we are creating multiple sessions, no?
There was a problem hiding this comment.
I'm treating the config sources as named entities. Let me try to clarify with a hypothetical example:
config_sources:
vault/db:
path: secret/db
vault/users:
path: secret/usersThen on usage, there should be one session for vault/db and another for vault/users when they are referenced on the config:
exporter:
db_exporter:
token: $vault/db:data.token
some_exporter:
username: $vault/users:data.traceruser
password: $vault/users:data.tracerpwd
pjanotti
force-pushed
the
add-config-source-manager
branch
from
82c4d1c to
e560941
Compare
|
Changed the single-line format to have the parameters like URL query part and add multi-line support. |
Currently, it is. I will fix it before hooking it up for actual usage. |
Adding support to existing env var syntax per #2857 (comment)
Adds a config source manager that wraps the interaction with config sources. This should be the part visible outside the configuration-related implementation, ie.: for the code setting up the service and watching for updates.
The syntax for config source references is provisional.