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

Jump to bottom

Feat: Upgrade from tags and fix issue with legacy IssuerRevRegRecords [<=v0.5.2] #2486

Merged
merged 1 commit into from
Sep 26, 2023
Merged

Feat: Upgrade from tags and fix issue with legacy IssuerRevRegRecords [<=v0.5.2] #2486

merged 1 commit into from
Sep 26, 2023

Conversation

shaangill025
Copy link
Contributor

@shaangill025 shaangill025 marked this pull request as ready for review September 11, 2023 16:28
@shaangill025 shaangill025 self-assigned this Sep 11, 2023
@shaangill025 shaangill025 removed their assignment Sep 11, 2023
esune
esune reviewed Sep 12, 2023
View reviewed changes
Copy link
Member

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

I think this looks good, waiting for @WadeBarnes feedback as he is more experienced with the upgrade process "on the field"

@WadeBarnes
Copy link
Contributor

I'd like to have this change in a separate release from 0.10.2 (#2479) as I think it's important to get those changes out first. I can build a pre-release image off this PR and use that for testing with the affected LSBC agent instance.

@swcurran
Copy link
Contributor

Definitely separate from 0.10.2. 0.10.2 is a patch release with two cherry picked PRs from main. This will go into what is likely 0.11.x when merged.

@shaangill025
Copy link
Contributor Author

shaangill025 commented Sep 14, 2023 •
edited
Loading

To run the upgrade:
./scripts/run_docker upgrade --force-upgrade --named-tag fix_issue_rev_reg

@WadeBarnes
Copy link
Contributor

@esune and @WadeBarnes to perform some testing before this gets merged.

@WadeBarnes
Copy link
Contributor

@esune, ghcr.io/hyperledger/aries-cloudagent-python:py3.9-indy-1.16.0-0.11.0-pre0-pr2486 has been built and contains the changes from this PR.

@WadeBarnes
Copy link
Contributor

WadeBarnes commented Sep 14, 2023 •
edited
Loading

@shaangill025, Error log from a test run on the LSBC dev agent:
#2485 (comment)

@WadeBarnes WadeBarnes mentioned this pull request Sep 14, 2023
Closed
@shaangill025
Copy link
Contributor Author

@WadeBarnes Should be fixed now, can you give another try?

@WadeBarnes
Copy link
Contributor

Test results from the latest changes can be found here (no errors); #2485 (comment)

@WadeBarnes
Copy link
Contributor

@shaangill025, I'm not sure the log messages should be issued at ERROR level when they aren't actually errors.

@WadeBarnes
Copy link
Contributor

WadeBarnes commented Sep 15, 2023 •
edited
Loading

There are still some issues with the calls to /revocation/registry/{rev_reg_id}/fix-revocation-entry-state however. Adding the details to the related ticket; #2485 (comment)

@shaangill025
Copy link
Contributor Author

@WadeBarnes If re-run the upgrade, are there any log messages related to IssueRevRegRecord?

@WadeBarnes
Copy link
Contributor

WadeBarnes commented Sep 15, 2023 •
edited
Loading

@WadeBarnes If re-run the upgrade, are there any log messages related to IssueRevRegRecord?

No. No output. I assumes that means it found nothing to do.

@shaangill025
Copy link
Contributor Author

No. No output. I assumes that means it found nothing to do.

Yes, no problematic records with issuance_type remain.

@WadeBarnes
Copy link
Contributor

Details of another error where added here; #2485 (comment). Not sure if this error is related to this issue or the fix at all.

@shaangill025
Copy link
Contributor Author

@WadeBarnes Can you please test this by calling fix-revocation-entry-state against the problematic rev_reg_ids?

@WadeBarnes
Copy link
Contributor

@WadeBarnes Can you please test this by calling fix-revocation-entry-state against the problematic rev_reg_ids?

I'm assuming I need to upgrade the instance with your latest changes on this PR first. Is that correct?

@shaangill025
Copy link
Contributor Author

I'm assuming I need to upgrade the instance with your latest changes on this PR first. Is that correct?

That's correct

@WadeBarnes
Copy link
Contributor

New pre-release image created with the latest changes to this PR; https://github.com/hyperledger/aries-cloudagent-python/pkgs/container/aries-cloudagent-python/129376415?tag=py3.9-indy-1.16.0-0.11.0-pr2486-pre2

@WadeBarnes
Copy link
Contributor

Encountered a different error. Details and logs here; #2485 (comment)

@WadeBarnes
Copy link
Contributor

After migrating from Indy to Askar storage the calls to fix-revocation-entry-state (with apply_ledger_update=false) completed successfully; #2485 (comment).

However the the call to fix-revocation-entry-state (with apply_ledger_update=true) failed; #2485 (comment)

@WadeBarnes
Copy link
Contributor

New pre-release image created with the latest changes to this PR; https://github.com/hyperledger/aries-cloudagent-python/pkgs/container/aries-cloudagent-python/129813006?tag=py3.9-indy-1.16.0-0.11.0-pr2486-pre3

@WadeBarnes
Copy link
Contributor

Latest changes fix the reported issues; #2485 (comment)

@WadeBarnes
Copy link
Contributor

We're going to have LSBC verify their revoke and re-issue workflow still works.

@WadeBarnes
Copy link
Contributor

LSBC has competed testing and confirmed things are working; #2485 (comment)

WadeBarnes
WadeBarnes requested changes Sep 20, 2023
View reviewed changes
Copy link
Contributor

@WadeBarnes WadeBarnes left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@shaangill025, The only request I have for this PR is for you to clean up the history a bit, and rebase it on the latest from main.

@shaangill025
parent 49e71c8
9b16660 
author Shaanjot Gill <[email protected]> 1694204418 +0530
committer Shaanjot Gill <[email protected]> 1695747717 +0530

parent 49e71c8
author Shaanjot Gill <[email protected]> 1694204418 +0530
committer Shaanjot Gill <[email protected]> 1695747613 +0530

parent 49e71c8
author Shaanjot Gill <[email protected]> 1694204418 +0530
committer Shaanjot Gill <[email protected]> 1695747425 +0530

parent 49e71c8
author Shaanjot Gill <[email protected]> 1694204418 +0530
committer Shaanjot Gill <[email protected]> 1695746776 +0530

tagged upgrade impl, fixed fix_ledger_entry function and added tests

Signed-off-by: Shaanjot Gill <[email protected]>
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@swcurran swcurran merged commit 28b7c3c into openwallet-foundation:main Sep 26, 2023
8 checks passed
@swcurran swcurran mentioned this pull request Sep 26, 2023
Closed
@WadeBarnes WadeBarnes mentioned this pull request Sep 28, 2023
Closed
27 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@esune esune esune left review comments

@WadeBarnes WadeBarnes WadeBarnes approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@shaangill025 @WadeBarnes @swcurran @esune