-
Notifications
You must be signed in to change notification settings - Fork 3
V1 Done and working #5
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
base: main
Are you sure you want to change the base?
Conversation
|
@DMiradakis Any update to my Pull Request? - is it obsolete by now? |
Fix foreign key constraint on FlowRun table
--- For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/Trubador/didact-engine?shareId=XXXX-XXXX-XXXX-XXXX).
Fix foreign key constraint issue in FlowRun table
|
|
- Introduced ExecutionMode entity and updated related configurations. - Added FlowRunRepository for managing flow runs. - Updated database context to include ExecutionModes DbSet. - Refactored database initialization logic to seed ExecutionModes. - Updated application to use SQLite as the default database provider. - Enhanced error handling and logging in various components.
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.
Pull Request Overview
This PR implements the initial V1 version of the Didact workflow engine with a complete working system. The engine transitions from SQL Server to SQLite for local development and adds core functionality for flow management, execution scheduling, and background processing.
Key changes:
- Migrated database provider from SQL Server to SQLite with proper configuration handling
- Implemented comprehensive flow and flow run management with repositories and controllers
- Enhanced task scheduling with efficient thread pool management and graceful shutdown capabilities
Reviewed Changes
Copilot reviewed 51 out of 69 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| DidactEngine/appsettings.json | Database configuration updated to support SQLite for local development |
| DidactEngine/TaskSchedulers/DidactThreadPoolScheduler.cs | Enhanced thread pool scheduler with proper synchronization and shutdown handling |
| DidactEngine/Services/FlowRunRepository.cs | New repository for flow run CRUD operations |
| DidactEngine/Services/FlowRepository.cs | New repository for flow management and configuration storage |
| DidactEngine/Services/Database/DatabaseInitializer.cs | Database initialization service with seeding capabilities |
| DidactEngine/Services/Contexts/DidactDbContext.cs | Database context updated for SQLite compatibility |
| DidactEngine/Program.cs | Application startup configuration with dependency injection setup |
Files not reviewed (17)
- .idea/.idea.DidactEngine/.idea/.gitignore: Language not supported
- .idea/.idea.DidactEngine/.idea/.name: Language not supported
- .idea/.idea.DidactEngine/.idea/indexLayout.xml: Language not supported
- .idea/.idea.DidactEngine/.idea/vcs.xml: Language not supported
- DidactEngine/Migrations/20230717051620_InitialSnapshot.Designer.cs: Language not supported
- DidactEngine/Migrations/20231205064918_RenamedStandardQueuesToHyperQueues.Designer.cs: Language not supported
- DidactEngine/Migrations/20231206055336_AddFlowRunsConfigurations.Designer.cs: Language not supported
- DidactEngine/Migrations/20231206071100_AddFlowRunsToQueueInbounds.Designer.cs: Language not supported
- DidactEngine/Migrations/20231209040238_RemoveFlowRefsFromQueueInboundConfigurations.Designer.cs: Language not supported
- DidactEngine/Migrations/20231209041434_AddWhoAuditColumnsToOrganization.Designer.cs: Language not supported
- DidactEngine/Migrations/20231209044908_AddBlockRunConfig.Designer.cs: Language not supported
- DidactEngine/Migrations/20231210080814_AddEngineConfiguration.Designer.cs: Language not supported
- DidactEngine/Migrations/20231210081256_AddEngineUniqueNameConstraint.Designer.cs: Language not supported
- DidactEngine/Migrations/20231210202046_AddFlowNameUniqueConstraint.Designer.cs: Language not supported
- DidactEngine/Migrations/20240107100141_AddTypeRefToFlow.Designer.cs: Language not supported
- DidactEngine/Migrations/20250604162806_InitialCreate.Designer.cs: Language not supported
- DidactEngine/Migrations/20250604165914_AddExecutionModesDbSet.Designer.cs: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| "AllowedHosts": "*", | ||
| "ConnectionStrings": { | ||
| "Didact": "Data Source=localhost;Initial Catalog=Didact;Integrated Security=true" | ||
| "Didact": "Data source=localhost;Initial Catalog=Didact;User Id=sa;Password=9b9i!B8PJrec56rEo@T8T3LB#5a3rnaKd@m?hBcP;" // running in docker sql server container |
Copilot
AI
Sep 14, 2025
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.
Database password is exposed in plain text in the configuration file. Consider using environment variables, user secrets, or a secure configuration provider for sensitive connection string information.
| "Didact": "Data source=localhost;Initial Catalog=Didact;User Id=sa;Password=9b9i!B8PJrec56rEo@T8T3LB#5a3rnaKd@m?hBcP;" // running in docker sql server container | |
| "Didact": "Data source=localhost;Initial Catalog=Didact;User Id=sa;Password=${DIDACT_DB_PASSWORD};" // running in docker sql server container; set DIDACT_DB_PASSWORD as an environment variable |
| private readonly ThreadLocal<string> _currentThreadName = new(); | ||
|
|
||
| private readonly int _maxDegreeOfParallelism; | ||
| private readonly ThreadLocal<bool> _currentThreadIsExecuting = new(false); private readonly ThreadLocal<string> _currentThreadName = new(); |
Copilot
AI
Sep 14, 2025
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.
[nitpick] Multiple field declarations should be on separate lines for better readability and maintainability.
| public DidactThreadPoolScheduler(ILogger<DidactThreadPoolScheduler> logger, int maxDegreeOfParallelism) | ||
| private readonly AutoResetEvent _workAvailable = new(false); | ||
|
|
||
| private volatile bool _shutdown = false; public DidactThreadPoolScheduler(ILogger<DidactThreadPoolScheduler> logger, int maxDegreeOfParallelism) |
Copilot
AI
Sep 14, 2025
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.
[nitpick] Field declaration and constructor should be on separate lines for better code readability.
| } | ||
|
|
||
| private void ThreadExecutionLoop() | ||
| } private void ThreadExecutionLoop() |
Copilot
AI
Sep 14, 2025
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.
[nitpick] Missing line break between method declarations. Add proper spacing for better code readability.
| // This blocks the thread instead of busy-waiting, dramatically reducing CPU usage | ||
| _workAvailable.WaitOne(1000); // Wait up to 1 second for work | ||
| } | ||
| } catch (ThreadInterruptedException ex) |
Copilot
AI
Sep 14, 2025
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.
[nitpick] Missing line break between closing brace and catch block. Add proper spacing for better code readability.
| private async Task SeedInitialDataAsync() | ||
| { // Add any initial seeding logic here | ||
| // For example, ensure there's at least one organization if (!await _dbContext.Organizations.AnyAsync()) |
Copilot
AI
Sep 14, 2025
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.
[nitpick] Inconsistent indentation and formatting in comments and code. Ensure proper line breaks and consistent spacing.
| { // Add any initial seeding logic here | ||
| // For example, ensure there's at least one organization if (!await _dbContext.Organizations.AnyAsync()) | ||
| { _dbContext.Organizations.Add(new DidactCore.Entities.Organization |
Copilot
AI
Sep 14, 2025
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.
[nitpick] Inconsistent indentation. The opening brace should be properly aligned with the if statement.
| { // Add any initial seeding logic here | |
| // For example, ensure there's at least one organization if (!await _dbContext.Organizations.AnyAsync()) | |
| { _dbContext.Organizations.Add(new DidactCore.Entities.Organization | |
| { | |
| // Add any initial seeding logic here | |
| // For example, ensure there's at least one organization | |
| if (!await _dbContext.Organizations.AnyAsync()) | |
| { | |
| _dbContext.Organizations.Add(new DidactCore.Entities.Organization |
| _flowExecutor = flowExecutor ?? throw new ArgumentNullException(nameof(flowExecutor)); | ||
| //_flowRepository = flowRepository ?? throw new ArgumentNullException(nameof(flowRepository)); | ||
| //_flowExecutor = flowExecutor ?? throw new ArgumentNullException(nameof(flowExecutor)); | ||
| } |
Copilot
AI
Sep 14, 2025
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.
Extra closing brace at the end of file that doesn't match any opening brace.
| using Microsoft.Extensions.DependencyInjection; | ||
| using DidactEngine.Services; | ||
| using Microsoft.Extensions.Hosting; | ||
| using Microsoft.Extensions.Hosting; |
Copilot
AI
Sep 14, 2025
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.
Duplicate using statement for Microsoft.Extensions.Hosting should be removed.
| using Microsoft.Extensions.Hosting; |
| /// </summary> | ||
| /// <returns></returns> | ||
| [HttpPost("/flows")] | ||
| [SwaggerResponse(StatusCodes.Status202Accepted)] public async Task<IActionResult> CreateFlowAsync() |
Copilot
AI
Sep 14, 2025
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.
[nitpick] Missing line break between attribute and method declaration. Add proper spacing for better code readability.
| [SwaggerResponse(StatusCodes.Status202Accepted)] public async Task<IActionResult> CreateFlowAsync() | |
| [SwaggerResponse(StatusCodes.Status202Accepted)] | |
| public async Task<IActionResult> CreateFlowAsync() |
No description provided.