3077 get otp from env #3121
3077 get otp from env #3121
Conversation
mssalvatore
requested review from
shreyamalviya and
VakarisZ
and removed request for
shreyamalviya
mssalvatore
force-pushed
the
3077-get-otp-from-env
branch
from
55e50d3
to
b8c7f3a
Compare
mssalvatore
force-pushed
the
3077-get-otp-from-env
branch
from
b8c7f3a
to
621a139
Compare
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## develop #3121 +/- ##
===========================================
+ Coverage 67.69% 71.73% +4.03%
===========================================
Files 449 449
Lines 12815 12828 +13
===========================================
+ Hits 8675 9202 +527
+ Misses 4140 3626 -514 see 23 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
|
|
||
| # SECURITY: There's no need to leave this floating around in a place as visible as | ||
| # environment variables for any longer than necessary. | ||
| del_key(os.environ, AGENT_OTP_ENVIRONMENT_VARIABLE) |
There was a problem hiding this comment.
I don't see a point in wrapping os.environ.pop(AGENT_OTP_ENVIRONMENT_VARIABLE, None). I think we shouldn't have del_key
There was a problem hiding this comment.
It's just defensive coding. If something else modifies the environment between these two statements (highly unlikely but ??), not using del_key() would cause the agent to crash.
| InfectionMonkey._get_otp() | ||
|
|
||
|
|
||
| def test_get_otp__feature_flag_disabled(monkeypatch): |
There was a problem hiding this comment.
We want to unit test feature flags? Interesting... We should come up with a feature flag naming convention, as FLAG is a term often used in cmd arguments and lower level code. Maybe F_FLAG? So it would be OTP_F_FLAG?
There was a problem hiding this comment.
It was easy enough to do it and it helped me quickly test that the feature flag was working as expected.
Environmental name for OTP is relevant to the island as well, it should be in an accessible place for both
What does this PR do?
Retrieve and remove the OTP from environment variables.
PR Checklist
Was the CHANGELOG.md updated to reflect the changes?Was the documentation framework updated to reflect the changes?Testing Checklist
Do all end-to-end tests pass?If applicable, add screenshots or log transcripts of the feature working