Ensure that EffectiveEnv and EffectiveArgs are properly updated during asynchronous Executable startup #60
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request fixes an issue where EffectiveEnv and EffectiveArgs properties of Executable resources might not be properly updated during asynchronous Executable startup.
Changes:
- Fixed the Executable controller to properly track and propagate status changes when computing effective environment and arguments, even during asynchronous startup
- Added test infrastructure (
TestProcessExecutableRunner) to simulate asynchronous executable startup with configurable delays - Extended existing integration tests to cover both synchronous and asynchronous startup scenarios
- Fixed a typo in a comment (Falback → Fallback)
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| controllers/executable_controller.go | Core fix: Changed computeExecutableEnvironment to return success flag and objectChange, ensuring EffectiveEnv/EffectiveArgs updates are propagated even during async startup; fixed typo |
| internal/exerunners/process_executable_runner.go | Fixed ordering: Set StartupError before calling OnStartupCompleted |
| internal/testutil/ctrlutil/test_process_executable_runner.go | New test wrapper to simulate asynchronous executable startup with configurable delays |
| internal/testutil/process.go | Moved process-related types from test_process_executor.go and added AsynchronousStartupDelay field |
| internal/testutil/test_process_executor.go | Removed process-related types that were moved to process.go |
| test/integration/executable_controller_test.go | Extended three tests to run with both sync and async startup scenarios |
| test/integration/standard_test_env.go | Integrated TestProcessExecutableRunner into test environment |
| test/integration/controllers_common_test.go | Added global testProcessExecutableRunner variable |
| test/integration/advanced_test_env.go | Updated to use TestProcessExecutableRunner and return AdvancedTestEnvironmentInfo |
| test/integration/container_network_tunnel_proxy_test.go | Updated to use new AdvancedTestEnvironmentInfo structure |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/backport to release/0.22 |
|
Started backporting to release/0.22: https://github.com/microsoft/dcp/actions/runs/21456865049 |
|
@karolz-ms backport PR couldn't be created automatically, please create the backport PR manually! |
A fix for an issue that I discovered when testing some Aspire updates. Basically, we might fail to update
EffectiveEnvandEffectiveArgsproperties during asynchronous Executable startup.Most of this change is about adding required test infra and tests to ensure we do not regress this in future. The actual fix is in the Executable controller.