Github Sync updates github docs parents Field #1296
Conversation
| @@ -281,7 +281,7 @@ export async function githubUpsertDiscussionActivity( | |||
| documentUrl: discussion.url, | |||
| timestampMs: new Date(discussion.createdAt).getTime(), | |||
| tags, | |||
| parents: [], | |||
| parents: [discussionNumber.toString(), repoId.toString()], | |||
There was a problem hiding this comment.
documentId should be visible here
There was a problem hiding this comment.
Otherwise we risk divergence?
There was a problem hiding this comment.
Actually maybe it's not documentid but at least should be a variable that is used to set the ressource internal Id ?
There was a problem hiding this comment.
Since no valid external id exist I will use the pre-existing getIssueDocumentId here and document it; same for discussion
There was a problem hiding this comment.
I will document it very explicitly
| @@ -154,7 +154,7 @@ export async function githubUpsertIssueActivity( | |||
| documentUrl: issue.url, | |||
| timestampMs: lastUpdateTimestamp, | |||
| tags: tags, | |||
| parents: [], | |||
| parents: [issueNumber.toString(), repoId.toString()], | |||
There was a problem hiding this comment.
Same here, documentId should be visible otherwise we risk divergence?
There was a problem hiding this comment.
Actually maybe it's not documentid but at least should be a variable that is used to set the ressource internal Id ?
| : await Connector.findAll({ | ||
| where: { | ||
| type: "github", | ||
| workspaceId: process.argv[2], |
There was a problem hiding this comment.
I guess it doesn't matter much since it a one off, but curious what happens if I forget argv[2] ? Does it basically does the same as "all" because workspaceId: undefined doesn't filter ?
There was a problem hiding this comment.
Nice catch. Let's validate that
| } | ||
|
|
||
| async function updateIssuesParentsFieldForConnector(connector: Connector) { | ||
| // get all distinct documentIds and their channel ids from slack messages in |
There was a problem hiding this comment.
nit: comment seems wrong
| }, | ||
| attributes: ["repoId", "issueNumber"], | ||
| }); | ||
| // update all parents fields for all pages and databases by chunks of 128 |
There was a problem hiding this comment.
nit: comment looks wrong
There was a problem hiding this comment.
You got me 😸 Draft wrote by copilot 🤖 and then I forgot to update the com
philipperolet
force-pushed
the
ds-parents-github
branch
from
b766ea5 to
de327b6
Compare
| // Therefore as a special case we use getIssueDocumentId() to get a parent string | ||
| parents: [ | ||
| getIssueDocumentId(repoId.toString(), issue.number), | ||
| repoId.toString(), |
There was a problem hiding this comment.
Just out of curiosity what does this Id look like? Is that a number?
There was a problem hiding this comment.
If we have 2 github managed data sources I worry we'll be clashing, which means we should probably update the connectors resource API to use something more specific?
We want to make sure it's globally unique
There was a problem hiding this comment.
No clear doc about this, but I have created two repos at ~ 30mn interval and their repo ids are 688834560 and 688870568
Pretty sure it's an autoincremented primary key
=> I am willing to decide it is globally unique, and be responsible for the consequences if i'm wrong
There was a problem hiding this comment.
These look long enough to me 👍 :)
| // Therefore as a special case we use getIssueDocumentId() to get a parent string | ||
| parents: [ | ||
| getIssueDocumentId(repoId.toString(), issue.number), | ||
| repoId.toString(), |
There was a problem hiding this comment.
If we have 2 github managed data sources I worry we'll be clashing, which means we should probably update the connectors resource API to use something more specific?
We want to make sure it's globally unique
philipperolet
dismissed stale reviews from spolu and fontanierh
The base branch was changed.
|
re last commit, you should use rebase instead of merging main 😬 |
|
Ok. I was using the Github UI "resolve conflicts" since it was a small conflict easily solvable, linked to Github autorebasing this branch on main when I merged its base branch. I thought it was ok in that case. Will systematically rebase from here on |
Based on the similar Notion PR for review simplicity (there is a common change)
Much, much, much simpler than Notion
Same migration plan