-
Notifications
You must be signed in to change notification settings - Fork 19
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
log model stream id #1176
log model stream id #1176
Conversation
f2a3ad2
to
bd01b20
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## develop #1176 +/- ##
===========================================
+ Coverage 81.56% 81.57% +0.01%
===========================================
Files 46 46
Lines 1801 1802 +1
Branches 293 276 -17
===========================================
+ Hits 1469 1470 +1
Misses 332 332
☔ View full report in Codecov by Sentry. |
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.
lgtm mod one small suggested test cleanup
@@ -109,7 +109,7 @@ export class RequestService { | |||
did, | |||
schema: genesisFields?.schema, | |||
family: genesisFields?.family, | |||
model: genesisFields?.model, | |||
model: genesisFields?.model?.toString() ?? '', |
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.
actually wait, I'm not sure this will actually log the right thing. Have you tested this with a real MID stream's genesis commit? I think you need to do StreamID.fromBytes(genesisFields.model).toString()
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'm not 100% positive though, so if you tested it with just calling toString on the Uint8Array and it printed the proper model streamid string we expect, then it's fine.
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.
No, so the thing is, because we decode it actually model is an object.
so, the structure of 'model' depends on whether it was run thru decode or not
carFile.get(genesisJWS.link).header.model
Uint8Array(40)
decode(IpfsGenesis, carFile.get(genesisJWS.link)).header.model
{ _tag: Symbol(@ceramicnetwork/streamid/StreamID),
_type: 2,
_cid: CID }
and in our case, it is using decode
const genesis = decode(IpfsGenesis, genesisRecord) |
const genesis = decode(IpfsGenesis, genesisRecord)
return genesis.header
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.
@stbrody fyi ^^ so i commited your change, will merge when it completes
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.
Ah I see, we use the codec to deserialize the genesis commit, so it's already a StreamID instance by the time we get here. Sounds good.
Co-authored-by: Spencer T Brody <[email protected]>
model stream id should be logged - #MET-1607
Description
The log data has been missing the model info. Upon examining a sample CARfile it seems that the genesisfields.header.model is an object, and we are trying to directly log it as a string. we need to extract the string.
Include relevant motivation, context, brief description and impact of the change(s). List follow-up tasks here.
How Has This Been Tested?
Describe the tests that you ran to verify your changes. Provide instructions for reproduction.
Definition of Done
Before submitting this PR, please make sure:
References:
Please list relevant documentation (e.g. tech specs, articles, related work etc.) relevant to this change, and note if the documentation has been updated.