-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
6181 workflows create a custom code executor #6235
6181 workflows create a custom code executor #6235
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
- Added dependencies for AWS Lambda and file handling in
package.json
- Introduced
CUSTOM_CODE_ENGINE_DRIVER_TYPE
in.env.example
- Integrated
FunctionModule
intocore-engine.module.ts
- Made
FileUploadModule
globally available infile-upload.module.ts
- Added
Function
entry toFileFolder
enum infile-folder.interface.ts
29 file(s) reviewed, 16 comment(s)
Edit PR Review Bot Settings
packages/twenty-server/src/engine/core-modules/function/function.module.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/core-modules/function/function.service.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/integrations/custom-code-engine/custom-code-engine.service.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/integrations/custom-code-engine/custom-code-engine.service.ts
Outdated
Show resolved
Hide resolved
...ne/integrations/custom-code-engine/drivers/interfaces/custom-code-engine-driver.interface.ts
Outdated
Show resolved
Hide resolved
if (code && code !== 0) { | ||
reject(new Error(`Child process exited with code ${code}`)); | ||
fs.unlink(tmpFilePath); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧠 logic: Ensure fs.unlink handles errors to avoid unhandled promise rejections.
packages/twenty-server/src/engine/integrations/custom-code-engine/utils/compile-typescript.ts
Outdated
Show resolved
Hide resolved
...y-server/src/engine/integrations/custom-code-engine/utils/temporary-lambda-folder-manager.ts
Outdated
Show resolved
Hide resolved
...y-server/src/engine/integrations/custom-code-engine/utils/temporary-lambda-folder-manager.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/integrations/file-storage/utils/read-file-content.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
(updates since last review)
- Fixed typos in import paths across multiple files for
FunctionWorkspaceEntity
- Implemented AWS Lambda driver for custom code execution (
lambda.driver.ts
) - Implemented local driver for custom code execution (
local.driver.ts
) - Added new fields and relations in
function.workspace-entity.ts
to support custom code executor - Ensured correct initialization of injected driver in
custom-code-engine.service.ts
9 file(s) reviewed, 6 comment(s)
Edit PR Review Bot Settings
this.fileUploadService = options.fileUploadService; | ||
} | ||
|
||
async upsert(file: FileUpload) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🪶 style: Consider adding error handling for file reading and TypeScript compilation.
} | ||
|
||
async upsert({ createReadStream, filename, mimetype }: FileUpload) { | ||
const typescriptCode = await readFileContent(createReadStream()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧠 logic: Ensure readFileContent
handles potential errors from the stream.
|
||
const { path: builtSourcePath } = await this.fileUploadService.uploadFile({ | ||
file: javascriptCode, | ||
filename: '.js', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🪶 style: Filename should include the original filename with a .js extension for clarity.
child.on('message', (message: object) => { | ||
resolve(message); | ||
child.kill(); | ||
fs.unlink(tmpFilePath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧠 logic: Ensure fs.unlink
handles errors to avoid unhandled promise rejections.
child.on('error', (error) => { | ||
reject(error); | ||
child.kill(); | ||
fs.unlink(tmpFilePath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧠 logic: Ensure fs.unlink
handles errors to avoid unhandled promise rejections.
if (code && code !== 0) { | ||
reject(new Error(`Child process exited with code ${code}`)); | ||
fs.unlink(tmpFilePath); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧠 logic: Ensure fs.unlink
handles errors to avoid unhandled promise rejections.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
(updates since last review)
- Removed JavaScript code upload in
packages/twenty-server/src/engine/integrations/custom-code-engine/drivers/lambda.driver.ts
- Set
builtSourcePath
toundefined
inLambdaDriver
class - Updated filename handling logic in
packages/twenty-server/src/engine/integrations/custom-code-engine/drivers/local.driver.ts
- Improved temporary file creation and cleanup in
local.driver.ts
2 file(s) reviewed, 6 comment(s)
Edit PR Review Bot Settings
packages/twenty-server/src/engine/integrations/custom-code-engine/drivers/lambda.driver.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/integrations/custom-code-engine/drivers/lambda.driver.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/integrations/custom-code-engine/drivers/local.driver.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/integrations/custom-code-engine/drivers/local.driver.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/integrations/custom-code-engine/drivers/local.driver.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/integrations/custom-code-engine/drivers/local.driver.ts
Outdated
Show resolved
Hide resolved
a99bc5f
to
48599a5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
(updates since last review)
- Introduced optional
builtSourcePath
incustom-code-engine-driver.interface.ts
- Removed JavaScript code upload in
lambda.driver.ts
- Set
builtSourcePath
toundefined
inLambdaDriver
class - Updated filename handling logic in
local.driver.ts
to ensure correct JavaScript filename generation - Handled upload of compiled JavaScript file with correct filename in
local.driver.ts
3 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings
packages/twenty-server/src/engine/integrations/custom-code-engine/drivers/lambda.driver.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
(updates since last review)
- Renamed
upsert
togenerateExecutable
inpackages/twenty-server/src/engine/core-modules/function/function.service.ts
- Updated
generateExecutable
method inpackages/twenty-server/src/engine/integrations/custom-code-engine/custom-code-engine.service.ts
- Modified
generateExecutable
method inpackages/twenty-server/src/engine/integrations/custom-code-engine/drivers/interfaces/custom-code-engine-driver.interface.ts
- Enhanced Lambda function deployment logic in
packages/twenty-server/src/engine/integrations/custom-code-engine/drivers/lambda.driver.ts
- Improved TypeScript compilation and execution in
packages/twenty-server/src/engine/integrations/custom-code-engine/drivers/local.driver.ts
5 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings
packages/twenty-server/src/engine/integrations/custom-code-engine/drivers/local.driver.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/integrations/custom-code-engine/drivers/local.driver.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/integrations/custom-code-engine/drivers/local.driver.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
(updates since last review)
- Modified
upsertFunction
to update existing functions by name inpackages/twenty-server/src/engine/core-modules/function/function.service.ts
- Minor refactoring for readability and maintainability in
packages/twenty-server/src/engine/core-modules/function/function.service.ts
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
packages/twenty-website/src/content/developers/self-hosting/self-hosting-var.mdx
Show resolved
Hide resolved
packages/twenty-server/src/modules/function/standard-objects/function.workspace-entity.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/modules/function/standard-objects/function.workspace-entity.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/modules/function/standard-objects/function.workspace-entity.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's impressive what you have made in a few days. Congrats!
I left a few comments about implementation details.
packages/twenty-server/src/engine/core-modules/code-engine/drivers/lambda.driver.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/core-modules/code-engine/drivers/lambda.driver.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/core-modules/code-engine/utils/build-directory-manager.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/core-modules/code-engine/utils/build-directory-manager.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/core-modules/code-engine/utils/create-zip-file.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/integrations/file-storage/utils/read-file-content.ts
Outdated
Show resolved
Hide resolved
...server/src/database/typeorm/metadata/migrations/1721040829256-createFunctionMetadataTable.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's great. Overall, I have 4 concerns:
- functionMetadata is not the right naming actually. Ideally it should be function but this is a keyword reserved in the language. So I think we should actually find something else: customFunction, functionBlock, codeFunction?
- code-engine is confusing with the twenty-engine. Maybe code-runner ? not a fan as we clearly have a build/compile and a run/execute phase
-,I think we should not pass the file-storage to the drivers - and drivers should be in the integrations folder
packages/twenty-server/src/engine/core-modules/code-engine/code-engine-module.factory.ts
Outdated
Show resolved
Hide resolved
...server/src/database/typeorm/metadata/migrations/1721040829256-createFunctionMetadataTable.ts
Outdated
Show resolved
Hide resolved
...server/src/database/typeorm/metadata/migrations/1721040829256-createFunctionMetadataTable.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/core-modules/code-engine/drivers/lambda.driver.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/core-modules/code-engine/utils/build-directory-manager.ts
Outdated
Show resolved
Hide resolved
...-server/src/engine/workspace-manager/workspace-sync-metadata/constants/standard-field-ids.ts
Outdated
Show resolved
Hide resolved
...server/src/engine/workspace-manager/workspace-sync-metadata/constants/standard-object-ids.ts
Outdated
Show resolved
Hide resolved
...rkspace-manager/workspace-sync-metadata/services/workspace-sync-function-metadata.service.ts
Outdated
Show resolved
Hide resolved
I don't like |
@charlesBochet @FelixMalfait what do you think about |
looks like we go for |
7aa6df7
to
b461901
Compare
b461901
to
fca54cb
Compare
272adc5
to
00be140
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another masterpiece!
Bravo @martmull !!!! |
Closes #6181
Testing
test.ts
containing:mutation UpsertFunction($file: Upload!) {
upsertFunction(name: "toto", file: $file)
}
mutation ExecFunction {
executeFunction(name:"toto", payload: {data: "titi"})
}