-
Notifications
You must be signed in to change notification settings - Fork 516
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
fix: Resolve Revocation Notification environment variable name collision #1751
fix: Resolve Revocation Notification environment variable name collision #1751
Conversation
`--monitor-revocation-notification` and `--notify-revocation` have a name conflict with their corresponding environment variables. Now they don't have a conflict. Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #1751 +/- ##
=======================================
Coverage 95.23% 95.23%
=======================================
Files 528 528
Lines 33141 33141
=======================================
Hits 31563 31563
Misses 1578 1578 |
aries_cloudagent/config/argparse.py
Outdated
@@ -680,7 +680,7 @@ def add_arguments(self, parser: ArgumentParser): | |||
parser.add_argument( | |||
"--monitor-revocation-notification", | |||
action="store_true", | |||
env_var="ACAPY_NOTIFY_REVOCATION", | |||
env_var="ACAPY_MONITOR_REVOCATION_NOTIF", |
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.
Nit, but I like it when the env variable follows the argument (so I could guess what the env name is from the arg name)
env_var="ACAPY_MONITOR_REVOCATION_NOTIF", | |
env_var="ACAPY_MONITOR_REVOCATION_NOTIFICATION", |
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.
Fair enough on you're point, let's make that change
aries_cloudagent/config/argparse.py
Outdated
@@ -680,7 +680,7 @@ def add_arguments(self, parser: ArgumentParser): | |||
parser.add_argument( | |||
"--monitor-revocation-notification", | |||
action="store_true", | |||
env_var="ACAPY_NOTIFY_REVOCATION", | |||
env_var="ACAPY_MONITOR_REVOCATION_NOTIF", |
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.
The abbreviation feels slightly out of place but it's also one of the longest monitoring subjects so I'm not feeling particularly nit-picky on that point
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.
That's the main reason why I shortened it, because it's such a long environment variable
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.
https://developer.apple.com/documentation/coremedia/1489454-cmmetadataformatdescriptioncreat I mean, could be worse .... 🙃
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
…cation-env-var-names Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
--monitor-revocation-notification
and--notify-revocation
have aname conflict with their corresponding environment variables. Now they
don't have a conflict.
Signed-off-by: Colton Wolkins (Indicio work address) [email protected]