-
Notifications
You must be signed in to change notification settings - Fork 242
Pass requestId to DX and remove backend_id from Operation #497
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
Conversation
peterbroadhurst
left a comment
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.
Looks good. Noting it has a co-req with hyperledger/firefly-dataexchange-https#54 meaning a hard dependency on upgrading DX with this.
Codecov Report
@@ Coverage Diff @@
## main #497 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 267 267
Lines 15240 15251 +11
=========================================
+ Hits 15240 15251 +11
Continue to review full report at Codecov.
|
|
Yep - good news is it won't fail anything without the change (as testified by the E2E tests here), but it will result in a bunch of DX operations being left "pending". Planning to merge this with a manifest.json change after the DX PR goes in. |
Follow the convention of all other plugins by passing a request ID into the connector, rather than reading one from the connector response. This will be enforced as a requirement for future connectors, so backend_id is removed from the operations table. Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
| Size int64 `json:"size"` | ||
| Error string `json:"error"` | ||
| Manifest string `json:"manifest"` | ||
| Info fftypes.JSONObject `json:"info"` |
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.
@peterbroadhurst @gabriel-indik I wanted to call special attention to this change. I don't actually see any dataexchange events that include "info" in their payload, so I don't think it breaks anything. Was this just added as a possible future plug point?
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.
@awrichar the Info field is actually expected. After reviewing the latest changes I realized this is something that is going to be coming from Data Exchange. This is, instead of having to deal with each individual type of response Data Exchange produces, the information will always be encapsulated under the "info" JSON object.
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Follow the convention of all other plugins by passing a request ID into the
connector, rather than reading one from the connector response. This will be
enforced as a requirement for future connectors, so backend_id is removed from
the operations table.
Depends on hyperledger/firefly-dataexchange-https#54