-
Notifications
You must be signed in to change notification settings - Fork 6
Process override env vars in launcher #458
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
base: feature_unification
Are you sure you want to change the base?
Process override env vars in launcher #458
Conversation
Signed-off-by: Mykola Kobets <mykola_kobets@epam.com>
Signed-off-by: Mykola Kobets <mykola_kobets@epam.com>
Signed-off-by: Mykola Kobets <mykola_kobets@epam.com>
Signed-off-by: Mykola Kobets <mykola_kobets@epam.com>
Signed-off-by: Mykola Kobets <mykola_kobets@epam.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feature_unification #458 +/- ##
=======================================================
+ Coverage 84.45% 84.48% +0.03%
=======================================================
Files 294 295 +1
Lines 24954 25113 +159
Branches 3410 3421 +11
=======================================================
+ Hits 21075 21217 +142
- Misses 3879 3896 +17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
| return ErrorEnum::eNone; | ||
| } | ||
|
|
||
| Error Balancer::ApplyEnvVarOverrides(aos::InstanceInfo& info) |
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.
Consider the next scenario:
- image config has no env vars set
- override env var handles ENV1=VAL1, TTL not set (and succeeds)
- override env var handles ENV2=VAL2, TTL not set (and succeeds)
What env vars are assigned to the instance after (3): both pairs?
| */ | ||
| bool mIsUnitSubject {}; | ||
|
|
||
| /** |
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.
I guess no need to comment each field in this struct. It makes this struct too big.
| return Error(); | ||
| } | ||
|
|
||
| Error SaveOverrideEnvVars(const OverrideEnvVarsRequest& envVars) override |
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.
Could you create pr on aos_core_cpp that implements these API?
| return var; | ||
| } | ||
|
|
||
| template <typename T> |
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.
It worth to be placed in common::tests::utils as well as same template from cryptoprovider test.


No description provided.