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

[content-nodes] Add a log and update the doc for lastUpdatedAt #9969

Merged
merged 5 commits into from
Jan 15, 2025

Conversation

aubin-tchoi
Copy link
Contributor

Description

  • Closes #9849.
  • This PR updates the documentation of /api/v1 to discourage the use of timestamp in upserted documents/tables.
  • This PR also adds a log to track user-specified timestamps.

Risk

  • Low.

Deploy Plan

  • Deploy front.
  • Update public documentation.

Copy link

github-actions bot commented Jan 14, 2025

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 2e9d9cc

@aubin-tchoi aubin-tchoi added the documentation-ack Used to acknowledge that documentation is up-to-date with this PR label Jan 14, 2025
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.

Maybe I missed something, but IMO we should log only if set; otherwise LGTM

@@ -308,6 +308,12 @@ async function handler(
mimeType = r.data.mime_type;
title = r.data.title;
} else {
// TODO(content-node): get rid of this once the use of timestamp columns in core has been rationalized
logger.info(
Copy link
Contributor

Choose a reason for hiding this comment

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

we should log only if set, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

also do we have the workspace id in the log? important if we need to warn them we change the field

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed the log if timestamp not set, added the workspace ID and dataSource sId

@@ -3296,8 +3296,8 @@
},
"/api/v1/w/{wId}/spaces": {
"get": {
"summary": "List Workspace Spaces",
"description": "Retrieves a list of spaces for the authenticated workspace.",
"summary": "List Spaces accessible.",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: english: "List available spaces"

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 👍 left a nit

@aubin-tchoi aubin-tchoi merged commit 3ab636d into main Jan 15, 2025
9 checks passed
@aubin-tchoi aubin-tchoi deleted the last-updated-at branch January 15, 2025 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation-ack Used to acknowledge that documentation is up-to-date with this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Core] Add lastUpdatedAt field to Node model
2 participants