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

feat(core): Remove storeMetadata and getSize from binary data manager interface (no-changelog) #7195

Merged
merged 172 commits into from
Sep 25, 2023

Conversation

ivov
Copy link
Contributor

@ivov ivov commented Sep 18, 2023

Depends on: #7164 | Story: PAY-838

This PR removes storeMetadata and getSize from the binary data manager interface, as these are specific to filesystem mode. Also this disambiguates identifiers:

binaryDataId
filesystem:289b4aac51e-dac6-4167-b793-6d5c415e2b47 {mode}:{fileId}

fileId - FS
289b4aac51e-dac6-4167-b793-6d5c415e2b47 {executionId}{uuid}

fileId - S3
/workflows/{workflowId}/executions/{executionId}/binary_data/b4aac51e-dac6-4167-b793-6d5c415e2b47

Note: The object store changes originally in this PR were extracted out into the final PR.

krynble
krynble previously approved these changes Sep 22, 2023
Copy link
Contributor

@krynble krynble left a comment

Choose a reason for hiding this comment

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

LGTM.

I fear this part of the code is untested, but it's something we should be tackling maybe as a separate PR, like tech debt. Especially the service should be tested once we settle on how it should look like so we avoid breaking it.

@cypress
Copy link

cypress bot commented Sep 22, 2023

2 flaky tests on run #2258 ↗︎

0 240 3 0 Flakiness 2

Details:

🌳 pay-769-implement-object-store-manager-for-binary-data 🖥️ browsers:node18.12...
Project: n8n Commit: c717e29c8d
Status: Passed Duration: 07:53 💡
Started: Sep 25, 2023 7:54 AM Ended: Sep 25, 2023 8:02 AM
Flakiness  12-canvas.cy.ts • 1 flaky test

View Output Video

Test Artifacts
Canvas Node Manipulation and Navigation > should disable node by pressing the disable button Output Screenshots Video
Flakiness  24-ndv-paired-item.cy.ts • 1 flaky test

View Output Video

Test Artifacts
NDV > resolves expression with default item when input node is not parent, while still pairing items Output Screenshots Video

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@github-actions
Copy link
Contributor

✅ All Cypress E2E specs passed

Base automatically changed from pay-768-generalize-binary-data-manager-interface to master September 22, 2023 15:22
@ivov ivov dismissed krynble’s stale review September 22, 2023 15:22

The base branch was changed.

@github-actions
Copy link
Contributor

Great PR! Please pay attention to the following items before merging:

Files matching packages/**:

  • If fixing bug, added test to cover scenario.
  • If addressing forum or Github issue, added link to description.

Files matching packages/**/*.ts:

  • Added unit tests to cover new or updated functionality.

Make sure to check off this list before asking for review.

@github-actions
Copy link
Contributor

✅ All Cypress E2E specs passed

@ivov ivov merged commit dcc9cc1 into master Sep 25, 2023
@ivov ivov deleted the pay-769-implement-object-store-manager-for-binary-data branch September 25, 2023 08:07
ivov added a commit that referenced this pull request Sep 25, 2023
…og) (#7220)

Depends on: #7195 | Story:
[PAY-837](https://linear.app/n8n/issue/PAY-837/implement-object-store-manager-for-binary-data)

This PR includes `workflowId` in binary data writes so that the S3
manager can support this filepath structure
`/workflows/{workflowId}/executions/{executionId}/binaryData/{binaryFilename}`
to easily delete binary data for workflows. Also all binary data service
and manager methods that take `workflowId` and `executionId` are made
consistent in arg order.

Note: `workflowId` is included in filesystem mode for compatibility with
the common interface, but `workflowId` will remain unused by filesystem
mode until we decide to restructure how this mode stores data.

---------

Co-authored-by: कारतोफ्फेलस्क्रिप्ट™ <aditya@netroy.in>
@janober
Copy link
Member

janober commented Sep 28, 2023

Got released with n8n@1.9.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team Released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants