feat: support multiple object storage + parquet_csv + polars -> datafusion" #3853
Conversation
Deploying windmill with
|
| Latest commit: |
3363385
|
| Status: | ⚡️ Build in progress... |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Incremental review on a4d72d3 in 2 minutes and 53 seconds
More details
- Looked at
23lines of code in2files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_Qus6mhyBxWsHiQZK
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| @@ -71,7 +71,7 @@ if [ "$REVERT" == "YES" ]; then | |||
| ce_file="${ee_file/${EE_CODE_DIR}/.}" | |||
| ce_file="${root_dirpath}/backend/${ce_file}" | |||
| if [ "$REVERT_PREVIOUS" == "YES" ]; then | |||
| git checkout HEAD@{5} ${ce_file} || true | |||
| git checkout HEAD@{3} ${ce_file} || true | |||
There was a problem hiding this comment.
Using git checkout HEAD@{3} assumes a specific state of the git history which might not be accurate and can lead to unexpected behavior. Consider using a more robust method to identify the commit to revert to, such as commit hashes or tags.
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 34200d8 in 5 minutes and 48 seconds
More details
- Looked at
2008lines of code in43files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. backend/windmill-api/openapi.yaml:7717
- Draft comment:
The summary for the endpoint/w/{workspace}/job_helpers/test_connectionhas been updated to reflect the support for multiple object storage options. Ensure that the backend implementation aligns with this description and correctly handles thestoragequery parameter. - Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 50%.
Workflow ID: wflow_y0kzL1OEpZJ59FmD
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 5e3979f in 4 minutes and 28 seconds
More details
- Looked at
672lines of code in26files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. backend/src/ee.rs:5
- Draft comment:
This function is always returning an error indicating non-usability in the community edition. Consider removing or ifdef-guarding this function to avoid confusion and potential dead code in community builds. - Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 50%.
2. backend/windmill-api/src/ee.rs:3
- Draft comment:
This function is always returning an error indicating non-usability in the community edition. Consider removing or ifdef-guarding this function to avoid confusion and potential dead code in community builds. - Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 50%.
3. backend/windmill-api/src/job_helpers_ee.rs:3
- Draft comment:
This function returns a new Router instance without any routes, which might indicate an incomplete implementation or oversight. Please ensure it serves its intended purpose or is properly documented. - Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 50%.
4. backend/windmill-api/src/oidc_ee.rs:11
- Draft comment:
This function returns a new Router instance without any routes, which might indicate an incomplete implementation or oversight. Please ensure it serves its intended purpose or is properly documented. - Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 50%.
5. backend/windmill-api/src/saml_ee.rs:18
- Draft comment:
This function sets up a route toacs, which always returns a string indicating non-availability in the community edition. Consider removing or ifdef-guarding this route to avoid confusion in community builds. - Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 50%.
6. backend/windmill-api/src/stripe_ee.rs:5
- Draft comment:
This function returns the input router without any modifications, which might indicate an incomplete implementation or oversight. Please ensure it serves its intended purpose or is properly documented. - Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 50%.
Workflow ID: wflow_Wfp4tr5Sp5So8lhh
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 299de5d in 3 minutes and 13 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. backend/windmill-worker/src/common.rs:1566
- Draft comment:
The change introduces a tuple assignment tostorageands3_etagswhere previously onlys3_etagswas set toNone. Ensure that this change is part of a larger set of changes that actually utilize these variables effectively, especially since the PR mentions support for multiple object storage options. - Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 50%.
Workflow ID: wflow_KcPgPQHnFdesiKhG
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 3363385 in 2 minutes and 1 seconds
More details
- Looked at
46lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. typescript-client/s3Types.ts:3
- Draft comment:
The addition of the optional 'storage' field in the S3Object type aligns with the PR's intent to support multiple object storage options. This change is correctly reflected in the client code where it's used. - Reason this comment was not posted:
Confidence changes required:0%
The PR description mentions support for multiple object storage options, which is reflected in the addition of an optional 'storage' field in the S3Object type. This change is consistent with the PR's intent. The code in 'client.ts' has been updated to handle this new field when making requests, which is a necessary change to support the new functionality. The 'DenoS3LightClientSettings' type has also been updated with optional fields, which seems to be a preparation for more flexible S3 client configuration. Overall, these changes align well with the PR's goals and there are no apparent issues with the implementation in this file.
Workflow ID: wflow_YmvixunlbmTkCimf
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Summary:
This PR introduces multiple object storage support, enhances Parquet and CSV handling, and integrates Polars with DataFusion, with comprehensive updates across backend, frontend, and clients.
Key points:
typescript-client/client.tsandtypescript-client/s3Types.tsto handle multiple storage optionsGenerated with ❤️ by ellipsis.dev