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.

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

[basicmessage_storage] Add message deletion and subwallet self disable. #58

Merged

Conversation

Jsyro
Copy link
Contributor

@Jsyro Jsyro commented Dec 14, 2023

bcgov/traction#903

Add endpoint to delete message by message id.

Add setting basicmessage_storage.wallet_enabled that allows subwallets to disable message storage.

Has unit tests, but integration tests are not setup for multi-tenancy, can be added later if required.

Signed-off-by: Jason Syrotuck <[email protected]>
Signed-off-by: Jason Syrotuck <[email protected]>
Signed-off-by: Jason Syrotuck <[email protected]>
Signed-off-by: Jason Syrotuck <[email protected]>
libindy not available inside devcontainer

Signed-off-by: Jason Syrotuck <[email protected]>
Signed-off-by: Jason Syrotuck <[email protected]>
Signed-off-by: Jason Syrotuck <[email protected]>
Signed-off-by: Jason Syrotuck <[email protected]>
Signed-off-by: Jason Syrotuck <[email protected]>
Signed-off-by: Jason Syrotuck <[email protected]>
Signed-off-by: Jason Syrotuck <[email protected]>
Signed-off-by: Jason Syrotuck <[email protected]>
Signed-off-by: Jason Syrotuck <[email protected]>
@Jsyro Jsyro marked this pull request as ready for review December 14, 2023 19:48
@Jsyro Jsyro requested review from jamshale and esune December 14, 2023 22:14
Copy link
Contributor

@jamshale jamshale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link

@esune esune left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. @Jsyro would there be a way to set the default storage behaviour to False (not store messages) and have each tenant opt-in, rather than the other way around?

@Jsyro
Copy link
Contributor Author

Jsyro commented Dec 18, 2023

Looks good to me. @Jsyro would there be a way to set the default storage behaviour to False (not store messages) and have each tenant opt-in, rather than the other way around?

Easy change, do we want it to always be false? or is should there be a top level config that is inherited by default?

This plugin does one thing, so I don't want have someone install it to miss there is also a config to enable.

@esune
Copy link

esune commented Dec 18, 2023

Looks good to me. @Jsyro would there be a way to set the default storage behaviour to False (not store messages) and have each tenant opt-in, rather than the other way around?

Easy change, do we want it to always be false? or is should there be a top level config that is inherited by default?

This plugin does one thing, so I don't want have someone install it to miss there is also a config to enable.

Top-level config seems like a good idea to me, it follow the patterns we have in place with startup parameters and other settings. The global plugin config would define the default behaviour, and then each tenant can override as-needed.

Signed-off-by: Jason Syrotuck <[email protected]>
Signed-off-by: Jason Syrotuck <[email protected]>
Signed-off-by: Jason Syrotuck <[email protected]>
Signed-off-by: Jason Syrotuck <[email protected]>
Signed-off-by: Jason Syrotuck <[email protected]>
@Jsyro
Copy link
Contributor Author

Jsyro commented Dec 18, 2023

Before I merge, this is a breaking change (without configuration changes to existing agents, their behaviour will change), are there corresponding instances/configurations that need to be updated?

@esune
Copy link

esune commented Dec 19, 2023

I don't think we are (yet) using the plugin code from this repo in Traction, and I am not aware of anybody else who would be using it either.

@jamshale jamshale merged commit 2b8d8a1 into openwallet-foundation:main Dec 20, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants