-
Notifications
You must be signed in to change notification settings - Fork 16
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
Push from aleph -> concat #147
Conversation
okay, I think this is in decent shape, although maybe we should merge #146 first, since this branches off of it. I could also do a bit of squashing to sand off some of the edges in the commit history 😄 The last commit adds an The node's publisher id is currently optional, which isn't ideal... it needs to be generated asynchronously, so you can't make one in the constructor. And I didn't want to make it mandatory since we'll need to update existing usages. The ingest command will fail with an error if it's not set. |
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.
Let's merge #146 first
.then(() => concatNodeClient()) | ||
.then(concat => concat.authorize(alephPeerIdB58, ['scratch.*'])) | ||
.catch(err => { | ||
console.error('error during push setup:', err) |
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.
do we really want to catch this?
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, sorry bout that! I stuck that in when things were failing during setup so I could print a stack trace - need to remove it. well spotted :)
this.db = Levelup(levelOpts) | ||
const location = (options.location == null || options.location === '') | ||
? '/aleph/data-' + uuid.v4() | ||
: options.location |
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.
const location = options.location || '/aleph/data-' + uuid.v4()
Unless you want number
0, NaN
and false
to be valid locations?
(there's a moment of wondering if js messes up the precedence in this but it's actually OK)
69b0763
to
eb1aa80
Compare
OK is this ready for final review? |
I think so; although I realized I don't have a test for a partially-successful push (some valid results + error). I think I'll add that real quick; hopefully it'll work as expected :) |
function seedStatementsToAleph (alephNode: AlephNode): Promise<Array<string>> { | ||
return Promise.all( | ||
seedObjects.map(obj => | ||
alephNode.ingestSimpleStatement('scratch.test', obj, { refs: [obj.id] }) |
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.
one comment here would be towards separating the test flow and the normal flow, i.e. not using ingest*
functions to seed test data, but this is probably a separate discussion around testing methodology
quick summary: