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

fix: c# on windows, oracledb test connection, cli for C# and Oracle DB #5090

Merged
merged 7 commits into from
Jan 17, 2025

Conversation

wendrul
Copy link
Contributor

@wendrul wendrul commented Jan 17, 2025

Important

Enhance C# and OracleDB support by updating script extensions, execution, and CLI functionalities, and improve OracleDB test connection handling.

  • Behavior:
    • Change OracleDB script extension from oracle.sql to odb.sql in workspaces_export.rs and script.ts.
    • Add C# script execution support in csharp_executor.rs, including error handling and result writing.
    • Update OracleDB test connection query in TestConnection.svelte to SELECT 1 FROM DUAL.
  • CLI:
    • Add C# and OracleDB support in script_common.ts, metadata.ts, and sync.ts.
    • Add C# script bootstrap code in script_bootstrap.ts.
  • WASM:
    • Add parse_csharp and parse_oracledb functions in windmill_parser_wasm.generated.js and windmill_parser_wasm.generated.d.ts.

This description was created by Ellipsis for 7e0ddd6. It will automatically update as commits are pushed.

@wendrul wendrul requested a review from rubenfiszel as a code owner January 17, 2025 18:18
Copy link

cloudflare-workers-and-pages bot commented Jan 17, 2025

Deploying windmill with  Cloudflare Pages  Cloudflare Pages

Latest commit: 80f6587
Status:⚡️  Build in progress...

View logs

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 2c680d4 in 1 minute and 4 seconds

More details
  • Looked at 689 lines of code in 11 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. frontend/src/lib/components/TestConnection.svelte:54
  • Draft comment:
    The change to SELECT 1 FROM DUAL for OracleDB is correct and necessary for compatibility with Oracle databases.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The code in TestConnection.svelte for OracleDB has been updated to use SELECT 1 FROM DUAL, which is correct for Oracle databases. This change is consistent with Oracle's requirement to select from a table, even if it's a dummy table like DUAL.

Workflow ID: wflow_S9rSLRwLOyqNJCYW


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@wendrul wendrul marked this pull request as draft January 17, 2025 18:20
@wendrul wendrul changed the title fix: c# on wiundws, oracledb test connection, cli for C# and Oracle DB fix: c# on windows, oracledb test connection, cli for C# and Oracle DB Jan 17, 2025
@wendrul wendrul marked this pull request as ready for review January 17, 2025 19:16
@rubenfiszel rubenfiszel merged commit 298aaae into main Jan 17, 2025
6 of 8 checks passed
@rubenfiszel rubenfiszel deleted the win-859-fix-csharp-on-windows branch January 17, 2025 19:16
@github-actions github-actions bot locked and limited conversation to collaborators Jan 17, 2025
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 7e0ddd6 in 2 minutes and 3 seconds

More details
  • Looked at 710 lines of code in 12 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. backend/windmill-api/src/workspaces_export.rs:358
  • Draft comment:
    The change from 'oracle.sql' to 'odb.sql' is consistent across the codebase, ensuring uniformity in file extensions for OracleDB scripts. This change is reflected in multiple files, including script.ts, script_common.ts, and sync.ts. The change is appropriate and aligns with the intent of the PR.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The change from 'oracle.sql' to 'odb.sql' is consistent across the codebase, ensuring uniformity in file extensions for OracleDB scripts. This change is reflected in multiple files, including script.ts, script_common.ts, and sync.ts. The change is appropriate and aligns with the intent of the PR.
2. backend/windmill-worker/src/csharp_executor.rs:290
  • Draft comment:
    The addition of error handling in the C# executor is a good practice. It ensures that unhandled exceptions are caught and logged, preventing the application from crashing silently. This change is beneficial for debugging and maintaining the application.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The addition of error handling in the C# executor is a good practice. It ensures that unhandled exceptions are caught and logged, preventing the application from crashing silently. This change is beneficial for debugging and maintaining the application.
3. backend/windmill-worker/src/csharp_executor.rs:341
  • Draft comment:
    The addition of the '-p:IncludeNativeLibrariesForSelfExtract=true' flag in the build command for C# projects is a good practice. It ensures that native libraries are included in the self-extracting executable, which is necessary for the application to run correctly on different environments.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The addition of the '-p:IncludeNativeLibrariesForSelfExtract=true' flag in the build command for C# projects is a good practice. It ensures that native libraries are included in the self-extracting executable, which is necessary for the application to run correctly on different environments.

Workflow ID: wflow_53X6Iacqn1eomJ5v


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants