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.

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

[parents_migration] Add and backfill Google Drive "Shared with me" folders #9507

Merged
merged 13 commits into from
Dec 19, 2024

Conversation

aubin-tchoi
Copy link
Contributor

@aubin-tchoi aubin-tchoi commented Dec 18, 2024

Description

  • Close Google Drive: create folders for "Shared With Me" #9502
  • AFAICT we never set GOOGLE_DRIVE_SHARED_WITH_ME_VIRTUAL_ID as a parentInternalId but it still works because we return the correct nodes when querying getPermissions with parent=GOOGLE_DRIVE_SHARED_WITH_ME_VIRTUAL_ID.

Risk

  • Lower than all the other parents migrations, easily rollbackable.

Deploy Plan

  • Deploy connectors.
  • Run migration.

@@ -71,6 +72,10 @@ export class GoogleDriveConnectorStrategy
},
transaction,
});
await deleteDataSourceFolder({
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this goes there
The Resource is not responsible for deleting stuff in core (i.e. it does not delete documents & tables too)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

didn't mean to put it here mb, moved it to the clean method of the ConnectorManager

Copy link
Contributor

@philipperolet philipperolet Dec 18, 2024

Choose a reason for hiding this comment

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

not sure it goes there either; although maybe it should
i'd say it goes where we also delete documents and tables and soon other folders
(note: logged the task here)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would put all these deletes here too I think, can't really think of another place

Copy link
Contributor

@philipperolet philipperolet left a comment

Choose a reason for hiding this comment

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

Looks good, left a com

@aubin-tchoi aubin-tchoi force-pushed the gdrive-shared-with-me branch from e239c85 to ebe0128 Compare December 18, 2024 17:12
Copy link
Contributor

@philipperolet philipperolet left a comment

Choose a reason for hiding this comment

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

Looks good 👍 Left 3 coms

@@ -129,6 +131,17 @@ export class GoogleDriveConnectorManager extends BaseConnectorManager<null> {
googleDriveConfigurationBlob
);

// Upserts to data_sources_folders (core) a top-level folder "Shared with me".
const folderId = getInternalId(GOOGLE_DRIVE_SHARED_WITH_ME_VIRTUAL_ID);
await upsertDataSourceFolder({
Copy link
Contributor

Choose a reason for hiding this comment

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

:cough: :cough: activity 😉

await concurrentExecutor(
connectors,
async (connector) => {
const folderId = getInternalId(GOOGLE_DRIVE_SHARED_WITH_ME_VIRTUAL_ID);
Copy link
Contributor

Choose a reason for hiding this comment

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

just checking: you told me documents already have this folderId in their parents right? (it was just not pointing to any real folder until this PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually we never pass this as a parentId, I kept this PR iso-behavior with main but I can't figure out how it used to work. I'll check again and fix it if needed

@philipperolet philipperolet self-requested a review December 19, 2024 12:46
@aubin-tchoi aubin-tchoi merged commit 5176705 into main Dec 19, 2024
3 checks passed
@aubin-tchoi aubin-tchoi deleted the gdrive-shared-with-me branch December 19, 2024 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Google Drive: create folders for "Shared With Me"
2 participants