-
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
chore: add indy deprecation warnings #2332
chore: add indy deprecation warnings #2332
Conversation
Signed-off-by: Daniel Bluhm <[email protected]>
cae6d7a
to
06263f8
Compare
@@ -1,25 +1,25 @@ | |||
"""Utilities related to logging.""" | |||
import asyncio | |||
from datetime import datetime, timedelta |
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.
These imports ought to be grouped to avoid re-ordering...
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've considered adding isort
or similar to pre-commit hooks and our format checks lol so it's handled automatically like black for formatting.
Co-authored-by: Andrew Whitehead <[email protected]> Signed-off-by: Daniel Bluhm <[email protected]>
I would be inclined to format the deprecation notice in a more attention grabbing fashion such as this (which is also keeping the formatting consistent with the start-up banner):
|
The link to the issue seems fine, but it should include an explicit example of what needs to change; startup parameter |
We also are going to add a document about how to migrate, and I’d like to use that as the target of the link — on the aca-py.org site. I’ll use the text that you put together the other day Wade about updating to Askar, along with some additional context. |
Signed-off-by: Daniel Bluhm <[email protected]>
The warning now looks like:
Happy to adjust the text whenever the document goes live! |
How about we assume that the link: https://aca-py.org/main/deploying/IndySDKtoAskarMigration/ is the link and I will make sure that the link exists by the time in conjunction with the merge? Do we want to put this into 0.9.0-rc0, or hold for 0.9.0-rc1/0.9.0 final (whichever comes first). I’m fine either way. |
Signed-off-by: Daniel Bluhm <[email protected]>
Signed-off-by: Daniel Bluhm <[email protected]>
Signed-off-by: Daniel Bluhm <[email protected]>
Kudos, SonarCloud Quality Gate passed! |
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.
Ready to go when the tests complete.
This PR adds deprecation warnings when using the Indy wallet type. The newly added warnings will look like this on startup, assuming wallet type indy is used and log level is set to at least
WARNING
:The
DEPRECATION NOTICE: The indy wallet type...
lines will always be printed regardless of log level tostderr
(but still only when using the indy wallet type).The lines added to the profile will ensure warnings are logged whenever a new sub-wallet is created in addition to the main wallet when operating without multi-tenancy. The
warnings.warn
lines show up in different circumstances from thelogging.warning
depending on how you're using ACA-Py. I really wanted something attention grabbing though so I added theprint_notices
to be printed immediately following the banner.Open to suggestions on what should be included in the warning or if we should point people to somewhere other than #2330.