Skip to content
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.

Sign up for GitHub

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

Merged
merged 5 commits into from
May 3, 2022
Merged

fix: Resolve Revocation Notification environment variable name collision #1751

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b6d45db
fix: Resolve Revocation Notif Env Name Collision
TheTechmage Apr 28, 2022
26dc478
Merge branch 'main' into fix/revocation-env-var-names
dbluhm May 3, 2022
43edcd2
fix: Expand variable to match argument's full name
TheTechmage May 3, 2022
4c764fb
Merge commit '43edcd2d25b31c75faac88901e94f7502375c5f7' into fix/revo…
TheTechmage May 3, 2022
6f43e9c
Merge branch 'main' into fix/revocation-env-var-names
dbluhm May 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion aries_cloudagent/config/argparse.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Copy link
Contributor

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)

Suggested change
env_var="ACAPY_MONITOR_REVOCATION_NOTIF",
env_var="ACAPY_MONITOR_REVOCATION_NOTIFICATION",

Copy link
Contributor Author

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

Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Contributor

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 .... 🙃

help=(
"Specifies that aca-py will emit webhooks on notification of "
"revocation received."
Expand Down