Skip to content
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.

Sign up for GitHub

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

9157 kw search update backfill folders for GitHub #9260

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

9157 kw search update backfill folders for GitHub #9260

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
d931d7b
first pass: upserting and deleting folders
Dec 10, 2024
96656d3
Added own id in parents
Dec 11, 2024
292aa6e
Applied connector-specific linter
Dec 11, 2024
4a24e67
Fixing hierarchy
Dec 11, 2024
d9fadcf
Handling Issues, Discussion and Code nodes
Dec 11, 2024
1668c9f
Moved logic outside of Workflow
Dec 11, 2024
7c435ae
Oversight
Dec 11, 2024
f489654
Wrote migration script for Github
Dec 12, 2024
3e3c308
Removed unnecessary logging
Dec 12, 2024
babaeaf
Better concern separation
Dec 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions connectors/src/connectors/github/lib/hierarchy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,9 @@ async function getGithubCodeDirectoryParentIds(
directory.parentInternalId,
repoId
);
return [directory.parentInternalId, ...parents];
return [directory.internalId, directory.parentInternalId, ...parents];
} else if (directory.parentInternalId === `github-code-${repoId}`) {
return [`github-code-${repoId}`, `${repoId}`];
return [directory.internalId, `github-code-${repoId}`, `${repoId}`];
overmode marked this conversation as resolved.
Show resolved Hide resolved
}
return [];
}
Expand Down Expand Up @@ -72,9 +72,9 @@ async function getGithubCodeFileParentIds(
file.parentInternalId,
repoId
);
return [file.parentInternalId, ...parents];
return [file.documentId, file.parentInternalId, ...parents];
} else if (file.parentInternalId === `github-code-${repoId}`) {
return [`${repoId}`, `github-code-${repoId}`];
return [file.documentId, `${repoId}`, `github-code-${repoId}`];
overmode marked this conversation as resolved.
Show resolved Hide resolved
}
return [];
}
48 changes: 48 additions & 0 deletions connectors/src/connectors/github/temporal/activities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,11 @@ import { newWebhookSignal } from "@connectors/connectors/github/temporal/signals
import { getCodeSyncWorkflowId } from "@connectors/connectors/github/temporal/utils";
import { dataSourceConfigFromConnector } from "@connectors/lib/api/data_source_config";
import {
deleteFolderNode,
deleteFromDataSource,
renderDocumentTitleAndContent,
renderMarkdownSection,
upsertFolderNode,
upsertToDatasource,
} from "@connectors/lib/data_sources";
import { ExternalOAuthTokenError } from "@connectors/lib/error";
Expand Down Expand Up @@ -668,6 +670,12 @@ export async function githubRepoGarbageCollectActivity(
repoId: repoId.toString(),
},
});

await deleteFolderNode({
dataSourceConfig,
folderId: repoId,
loggerArgs: logger.bindings(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 points here:

  1. what led you to pass logger.bindings here? not sure what this does
  2. in general, this argument is now not necessary, you can remove it from the call and even delete it from deleteFolderNode

Copy link
Contributor Author

@overmode overmode Dec 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added it because it is used in upsertDataSource. Indeed currently not used in deleteFolderNode (although it is a parameter), but thought that if we start using it, at least we don't have to modify this call to pass it. I'm ok with deleting it 👍

});
}

async function deleteIssue(
Expand Down Expand Up @@ -846,6 +854,19 @@ async function garbageCollectCodeSync(
},
},
});

// Also delete data source folders
const fq = new PQueue({ concurrency: 8 });
overmode marked this conversation as resolved.
Show resolved Hide resolved
directoriesToDelete.forEach((d) =>
fq.add(async () => {
Context.current().heartbeat();
await deleteFolderNode({
dataSourceConfig,
folderId: d.internalId,
loggerArgs: logger.bindings(),
});
})
);
}
}

Expand Down Expand Up @@ -910,6 +931,12 @@ export async function githubCodeSyncActivity({
},
});

await deleteFolderNode({
dataSourceConfig,
folderId: repoId.toString(),
loggerArgs: logger.bindings(),
});

return;
}

Expand Down Expand Up @@ -941,6 +968,14 @@ export async function githubCodeSyncActivity({
githubCodeRepository.lastSeenAt = codeSyncStartedAt;
await githubCodeRepository.save();

// Add as dataSource folder
await upsertFolderNode({
dataSourceConfig,
folderId: githubCodeRepository.repoId,
title: githubCodeRepository.repoName,
parents: [githubCodeRepository.repoId],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't forget, parents[0] === folder_id as you changed in connectors/src/connectors/github/lib/hierarchy.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case we are uploading a node for the repository itself, which is at root level, that's why it is its own id

});

logger.info(
{
repoId,
Expand Down Expand Up @@ -991,6 +1026,12 @@ export async function githubCodeSyncActivity({
},
});

await deleteFolderNode({
dataSourceConfig,
folderId: repoId.toString(),
loggerArgs: logger.bindings(),
});

return;
}

Expand Down Expand Up @@ -1167,6 +1208,13 @@ export async function githubCodeSyncActivity({
});
}

await upsertFolderNode({
dataSourceConfig,
folderId: d.internalId,
parents: [d.internalId, ...d.parents, repoId.toString()],
title: d.dirName,
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are missing the intermediate "github-code-" thing, which does not exist in connectors database, but does exists in d.parents.
I also did not check the issues/discussion structure but i think we also have intermediate virtual folders here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(no structure for issues, it's just a big database entry)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new commit handles the "Code", "Issues" and "Discussions" nodes


// If the parents have updated then the internalId gets updated as well so we should never
// have an udpate to parentInternalId. We check that this is always the case. If the
// directory is moved (the parents change) then it will trigger the creation of a new
Expand Down
Loading