Repository navigation
Skip certificate trust preparation for HTTP-only Aspire test resources - #7618
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 24 minutes. View limit details
No actionable comments were generated in the recent review. Walkthrough
Merge Risk:
|
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
ReviewThe change is small and consistent with the existing crashed-executable probes, which already use Notes:
No blocking issues. LGTM. |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
[Low impact] The PR appears safe to merge, with no actionable new issues found. SummaryThe PR skips custom certificate trust preparation for the two HTTP-only Aspire test resources.
No actionable new issues were found. Reviews (2) · Last reviewed commit: "Capture child process exit diagnostics f..." · Reviewed by Greptile |
|
Both required reviewers approved head ba75eb4 with no blocking findings, and there are no review threads. The startup change remains a mitigation whose Ubuntu validation must come from CI; no helper is warranted for these two call sites. Marking ready to run CI. Auto-merge must wait for the Claude review triggered by promotion. |
ReviewTwo-line change in the test AppHost: Notes:
No blocking issues. LGTM. |
|
Run 41949899257 completed successfully; both required reviewers approved ba75eb4 without blockers. Ubuntu .NET CI passed, including the Aspire fixture. The remaining failure in run 41949898636 is macOS Reflection: Bug7126CultureInvariantDisplayNameTests(de-DE) exited after 153ms with no TRX or captured output. Its diagnostic artifact contains only two successful sibling culture sessions, not the failed child. This matches the early-child failure under investigation in #7607; an unchanged-head rerun is not justified by the available evidence. Pushed 4915a0e to record PID, exit code (or explicitly unavailable), expected TRX path, and the original exception on child failures. This is evidence collection for #7607, not a claimed root-cause fix. Built the modified Engine.Tests project on fresh main with SDK 10.0.403: 0 errors (existing warnings); did not execute Engine.Tests locally. git diff --check passed. Fresh hosted CI and reviews must assess this head before merge; auto-merge is disabled. |
ReviewThe change looks good. AppHost (
Two notes, neither blocking:
No correctness, design or AOT concerns. |
|
The earlier wait on run 41949899257 has cleared. Current head 4915a0e has positive Claude and Codex verdicts, no review threads or inline findings, and all CI checks green. Ubuntu run 41953665808 passed all 180 Aspire tests on net10.0 with SDK 10.0.403, satisfying the requested hosted evidence without changing timeouts or assertions. The earlier head also passed Ubuntu CI, as recorded above; this is mitigation evidence, not proof that an intermittent failure can never recur. Updated the description to cover the engine child-process diagnostics and validation, addressing Claude's remaining documentation note. The existing #7607 tracks the separate child-process failure. Proceeding through the merge gate. |
The shared Aspire AppHost can time out before its HTTP resources start on Ubuntu. Disable custom certificate trust preparation for its nginx container and API project, matching the crashed-executable probes: neither resource needs custom CA trust, and the API receives HTTP requests and exports OTLP over HTTP.
In the original attempt of run 41928218765, both shared resources remained
unknownthroughoutStartAsync, while the probes withCertificateTrustScope.Nonecompleted. The later resource-removal fixture also started successfully. The missing parameter is intentional coverage and also appears in successful runs; Aspire initializes it without waiting for resolution. Certificate preparation is an unnecessary startup dependency for these HTTP-only resources. This change removes it without extending the timeout, retrying startup, or weakening readiness, telemetry, or crashed-process assertions. The intermittent Ubuntu failure was not reproduced locally, so CI must confirm the mitigation there.The engine-test harness also records child PID, exit code (or explicitly unavailable), and expected TRX path, preserving the original exception as the inner exception when child execution fails. This collects evidence for #7607 after the previous macOS culture-test child exited without captured output; it does not claim to fix that separate failure.
Validation:
dotnet test --project tests/TUnit.Aspire.Tests/TUnit.Aspire.Tests.csproj --framework net10.0passed all 180 tests on macOS with SDK 10.0.403 (baseline: 180 passed; changed: 180 passed).git diff --checkpassed. No shipped API changes.Hosted validation on current head
4915a0e8761334546b77ff807ccccc34b22dd568: all checks passed, including Ubuntu Aspire (180/180 tests, net10.0, SDK 10.0.403), macOS pipeline, and Ubuntu/Windows Engine tests and AOT. Ubuntu pipeline evidence. The modified Engine.Tests project was built locally; Engine.Tests execution was left to CI per repository guidance.Closes #7616
Summary by CodeRabbit