-
Notifications
You must be signed in to change notification settings - Fork 135
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
feat: Upgrade SDK v0.47 to v0.50 #1069
Conversation
41ee174
to
95227b2
Compare
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.
Blocking this until I find some time to step through all the changes. Other than code review, there's a few things missing here:
- Rebase on latest version of
master
- PR description including short description & already performed tests
- before these changes are added to master, we need full E2E testing including compatibility with Paloma's side loading app Pigeon. We can compile a list of manual testing steps required to pass successfully before we consider going ahead.
How shall we proceed with E2E testing with Paloma's Pigeon App? |
I suggest Volume compiles a list of manual testing steps and required proof of verification to share with the vitwit team. Happy to drive this on Notion until we settle on a run book. |
4a3db48
to
bcdf4d2
Compare
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.
Some questions regarding proto changes.
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.
1st round of feedback
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.
Only small stuff remaining, but most of it we'll just accept for now.
Can you please also run a full scan over the code base and make sure there's no other occurrances of CacheContext()
that slipped through the renaming?
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.
Final review to @aleem1314
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.
pre approving
@aleem1314 we have just TWO unresolved convos on the PR. We will merge on your check and resolve. |
Just wanted to say that I thought this is cool :-) |
That means so much to us @faddat! Thank you! |
Related Github tickets
Background
This PR contains all the changes that has to be done for the upgrade, including integration tests/unit tests. This also contains latest master branch commits upto this
Testing completed
keeper_integration_tests
are moved totests/integration/{module}
with local fixture setup, leaving the dependency on app/app_setup.go (so the file app_setup.go is deleted)putMessageInQueue
in the casethere is another upload valset already in
only but later we needed to add for two more functions,TestFirstSnapshot_OnSnapshotBuilt()
function andTestOldPublishedSnapshot_OnSnapshotBuilt
functionlibmeta.getSigner
but the test cases were failing so I mentioned here with comment, you may interpret this change to adapt with the test case.Breaking changes