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

Jump to bottom

[content-nodes] Fill provider_visibility in data_source_nodes from connectors -> api/v1 #9988

Merged
merged 4 commits into from
Jan 16, 2025
Merged

[content-nodes] Fill provider_visibility in data_source_nodes from connectors -> api/v1 #9988

merged 4 commits into from
Jan 16, 2025

Conversation

aubin-tchoi
Copy link
Contributor

Description

  • Closes #9952
  • This PR fills the providerVisibility, passing from connectors to /api/v1 and ultimately to core (which already supports it in its API).
  • It also adds a type for it in types instead of using "private" | "public" repeatedly.
  • Tested locally on Slack (only connector affected).

Risk

  • Low.

Deploy Plan

  • Deploy front.
  • Deploy connectors.

@aubin-tchoi aubin-tchoi changed the title [content-nodes] Fill provider_visibility in data_source_nodes [content-nodes] Fill provider_visibility in data_source_nodes from connectors -> api/v1 Jan 15, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 15, 2025 •
edited
Loading

Warnings
⚠️

Files in **/api/v1/ have been modified and the PR has the documentation-ack label.
Don't forget to run npm run docs and use the Deploy OpenAPI Docs Github action to update https://docs.dust.tt/reference.

Generated by 🚫 dangerJS against beabc90

@aubin-tchoi aubin-tchoi added the documentation-ack Used to acknowledge that documentation is up-to-date with this PR label Jan 15, 2025
@aubin-tchoi aubin-tchoi force-pushed the fill-providerVisibility branch from 4fdf933 to b7371f8 Compare January 15, 2025 14:55
philipperolet
philipperolet reviewed Jan 16, 2025
View reviewed changes
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.

LGTM; nit on deriving type from the schema

types/src/front/lib/connectors_api.ts
@@ -56,6 +56,7 @@ export type ConnectorType = {
*/
export type ConnectorPermission = "read" | "write" | "read_write" | "none";
export type ContentNodeType = "file" | "folder" | "database" | "channel";
export type ProviderVisibility = "public" | "private";
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: if you have a schema, the type should be derived from the schema

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 schema lives in sdks/js, it seems that types and sdks/js don't import stuff from one another and that we duplicate types (e.g. ConnectorProvider is here in sdks/js but also here in types)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

kinda feel like we have to duplicate out of consistency

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed the SDK declares it's own types and they should not be used outside of the SDK itself : they define the public API contract.
By checking that internal types keeps matching the public types (because they are used as output of v1/ enpoints), we ensure that we don't break anything.

Does that make sense ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes very clear, thanks!

@aubin-tchoi aubin-tchoi force-pushed the fill-providerVisibility branch from b7371f8 to 1d6e1ba Compare January 16, 2025 11:08
@aubin-tchoi aubin-tchoi force-pushed the fill-providerVisibility branch from 1d6e1ba to beabc90 Compare January 16, 2025 11:15
@aubin-tchoi aubin-tchoi added the sdk-ack Used to acknowledge that you are not breaking the public API. label Jan 16, 2025
@aubin-tchoi aubin-tchoi requested a review from Fraggle January 16, 2025 11:16
@aubin-tchoi
Copy link
Contributor Author

tagged @Fraggle because I got a scary dangerJs regarding sdks js

@philipperolet
Copy link
Contributor

tagged @Fraggle because I got a scary dangerJs regarding sdks js

That's fine IMO because you're acting on an undocumented endpoint that should not be used, even by the extension

You can merge, we can get Fraggle's opinion after and course correct--which I'm confident we won't have to & I'm taking responsibility

@philipperolet philipperolet self-requested a review January 16, 2025 11:24
@Fraggle
Copy link
Contributor

Fraggle commented Jan 16, 2025

tagged @Fraggle because I got a scary dangerJs regarding sdks js

That's fine IMO because you're acting on an undocumented endpoint that should not be used, even by the extension

It's double fine:

  1. it's an undeclared endpoint
  2. you are adding a property, it's okay (you are backward compatible)

@aubin-tchoi aubin-tchoi merged commit a24230d into main Jan 16, 2025
8 checks passed
@aubin-tchoi aubin-tchoi deleted the fill-providerVisibility branch January 16, 2025 12:14
@aubin-tchoi aubin-tchoi mentioned this pull request Jan 16, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@philipperolet philipperolet philipperolet approved these changes

@Fraggle Fraggle Awaiting requested review from Fraggle

Assignees
No one assigned
Labels
documentation-ack Used to acknowledge that documentation is up-to-date with this PR sdk-ack Used to acknowledge that you are not breaking the public API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fill and backfill provider_visibility properly in core from connectors
3 participants
@aubin-tchoi @philipperolet @Fraggle