feat: Add AppConfig parameter provider #236
Conversation
There was a problem hiding this comment.
Hey Ran!
Thanks a lot for your PR! Overall, from an UX perspective, I just have two main questions:
- Should we rename the helper function to
get_app_config()instead? - Should we allow developers to control the client ID?
I've also flagged 2-3 things just for tracking purposes (e.g examples, a todo).
| "BaseProvider", | ||
| "GetParameterError", | ||
| "DynamoDBProvider", | ||
| "SecretsProvider", | ||
| "SSMProvider", | ||
| "TransformParameterError", | ||
| "get_configuration", |
There was a problem hiding this comment.
I'm wondering if this could cause confusion. Should this be get_app_config?
There was a problem hiding this comment.
sure, good idea.
|
|
||
| from .base import DEFAULT_PROVIDERS, BaseProvider | ||
|
|
||
| CLIENT_ID = str(uuid4()) |
There was a problem hiding this comment.
Question: should we keep this configurable when initializing the class?
There was a problem hiding this comment.
is that auditable in some way ? if not, i suppose it's best to keep things simple.
There was a problem hiding this comment.
I think this is a good default, but I'm wondering if there could be cases where someone might want to share a client ID across multiple execution environment.
ran-isenberg
force-pushed
the
appconf
branch
2 times, most recently
from
e1091f1 to
1df31c1
Compare
ran-isenberg
changed the title
ran-isenberg
force-pushed
the
appconf
branch
from
1df31c1 to
9515d50
Compare
Codecov Report
@@ Coverage Diff @@
## develop #236 +/- ##
===========================================
- Coverage 99.81% 99.71% -0.11%
===========================================
Files 76 77 +1
Lines 2768 2804 +36
Branches 112 113 +1
===========================================
+ Hits 2763 2796 +33
- Misses 4 6 +2
- Partials 1 2 +1
Continue to review full report at Codecov.
|
|
@heitorlessa @nmoutschen added docs and more tests. Please review :) |
ran-isenberg
force-pushed
the
appconf
branch
from
9515d50 to
b8a5b25
Compare
|
I know Ran is currently ill (wishing him a speedy recovery), so I'll wait until Friday morning before we send those fixes ourselves to ensure this gets shipped in 1.10.0 this week -- ;) |
|
@nmoutschen can we merge to release 1.10 with this feat? |
|
Yep! LGTM here. I'll let you do it since I pushed changes. |
* develop: chore: move env names to constant file (#264) docs: fix import (#267) feat: Add AppConfig parameter provider (#236) chore: update stale bot improv: override Tracer auto-capture response/exception via env vars (#259) docs: add info about extras layer (#260) feat: support extra parameter in Logger messages (#257) chore: general simplifications and cleanup (#255)
#187
this is a poc for the app conf provider.
there's a lot of work in the docs department and in the tests.
There's just one test at the moment but it works.
There's also the issue of handling yaml confs. This might require a new transform with pyyaml package maybe.
Is the UX ok?