feat(aspire): add ability to manually remove resources - #5586
Conversation
Up to standards ✅🟢 Issues |
| Metric | Results |
|---|---|
| Complexity | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewerTIP This summary will be updated as you push new changes.
e40e797 to
1365afaCompareThere was a problem hiding this comment.
Code Review
This is a clean, well-motivated addition — removing UI/observability resources (pgAdmin, kafka-ui, etc.) before the app starts is a common need and the declarative API fits well alongside ResourcesToWaitFor().
Issue 1: RemoveResources is called before ConfigureBuilder — ordering should be reversed
In the current PR, the sequence is:
varbuilder=awaitDistributedApplicationTestingBuilder.CreateAsync<TAppHost>(Args,ConfigureAppHost);RemoveResources(builder);// ← removalConfigureBuilder(builder);// ← user customisationThis means a user who adds a resource inside ConfigureBuilder (e.g. a mock service injected only during tests) cannot remove it via ResourcesToRemove() — the removal has already run. Calling RemoveResourcesafterConfigureBuilder is strictly more flexible and doesn't break the primary use-case:
varbuilder=awaitDistributedApplicationTestingBuilder.CreateAsync<TAppHost>(Args,ConfigureAppHost);ConfigureBuilder(builder);RemoveResources(builder);// sees both AppHost and ConfigureBuilder resourcesThe comment on the OTLP block (// Configure OTLP endpoint on project resources AFTER user's ConfigureBuilder) shows this ordering principle is already established — removal should follow the same convention.
Issue 2: Missing LogProgress calls — silent failures are hard to debug
Every other lifecycle step in InitializeAsync emits a LogProgress line. RemoveResources is completely silent: if a name is misspelled or the resource doesn't exist, nothing indicates it was skipped. Suggested addition:
privatevoidRemoveResources(IDistributedApplicationTestingBuilderbuilder){foreach(varnameinResourcesToRemove()){varresource=builder.Resources.SingleOrDefault(r =>string.Equals(r.Name,name,StringComparison.Ordinal));if(resourceis not null){builder.Resources.Remove(resource);LogProgress($"Removed resource '{name}' from builder.");}else{LogProgress($"ResourcesToRemove: resource '{name}' not found in builder (skipped).");}}}This surfaces typos in resource names during test runs without changing behaviour.
Issue 3: SingleOrDefault can throw on duplicate resource names
While duplicate names are unusual in Aspire, SingleOrDefault throws InvalidOperationException if two resources share a name. FirstOrDefault is safer here:
varresource=builder.Resources.FirstOrDefault(r =>string.Equals(r.Name,name,StringComparison.Ordinal));What's good
- The feature fills a real gap cleanly.
- The virtual method pattern (
ResourcesToRemove() => []) is consistent withResourcesToWaitFor(). - Tests cover both the removed resource being absent and the other resources being unaffected — good boundary testing.
- Documentation is thorough, includes a FAQ update, and clearly explains the case-sensitive matching behaviour.
The two ordering and logging issues are worth fixing before merge; the SingleOrDefault → FirstOrDefault change is low-risk but worth doing for robustness.
1365afa to
bae432eCompareThere was a problem hiding this comment.
Code Review — Follow-up
Comparing this version against the previous review: the three raised issues are not yet resolved. Flagging them again for visibility.
Issue 1 (unaddressed): Ordering — RemoveResources should run after ConfigureBuilder
The call sequence is still:
varbuilder=awaitDistributedApplicationTestingBuilder.CreateAsync<TAppHost>(...);RemoveResources(builder);// ← runs firstConfigureBuilder(builder);// ← user customisation runs afterIf a user adds a resource inside ConfigureBuilder (e.g. a test-only mock service) and also lists it in ResourcesToRemove(), the removal silently has no effect because it already ran. Swapping the order costs nothing and makes the API consistent with the existing OTLP comment in InitializeAsync that says "AFTER user's ConfigureBuilder":
ConfigureBuilder(builder);RemoveResources(builder);// sees both AppHost resources and test-injected onesIssue 2 (partially addressed): No LogProgress on successful removal
The "not found" log was added — good. But the success path is still silent, so during a test run there's no confirmation that the removal actually happened:
if(resourceis not null){builder.Resources.Remove(resource);LogProgress($"Removed resource '{name}' from distributed application.");// ← add this}else{LogProgress($"ResourcesToRemove: resource '{name}' not found (skipped).");}Every other lifecycle step in InitializeAsync emits a progress line; removal should too.
Issue 3 (unaddressed): SingleOrDefault can throw on duplicate names
varresource=builder.Resources.SingleOrDefault(r =>string.Equals(r.Name,name,StringComparison.Ordinal));If two resources share a name (unusual but possible in programmatically built apps), this throws InvalidOperationException. FirstOrDefault is drop-in safer:
varresource=builder.Resources.FirstOrDefault(r =>string.Equals(r.Name,name,StringComparison.Ordinal));What's good
The feature itself is solid — clean virtual-method pattern consistent with ResourcesToWaitFor(), good test coverage (removed resource absent + other resources still present), and thorough documentation including a FAQ update. All three issues above are small fixes; none require design changes.
thomhurst
commented
Apr 21, 2026
@Odonno are you okay to take a look at claude's suggestions? |
Odonno
commented
Apr 21, 2026
Already applied some. I don't really agree on 2 and 3. For 1, I don't really know. I had the assumption that anyone could override |
thomhurst
commented
Apr 22, 2026
I think Issue 1 we should do, the others can be ignored |
bae432e to
5da4ce4CompareOdonno
commented
Apr 23, 2026
Done. |
5da4ce4 to
8a3381eComparethomhurst
commented
Apr 26, 2026
Thanks 😄 |
Uh oh!
There was an error while loading. Please reload this page.
Description
The goal is to be able to removes specific resources that one knows should not be present/created during a test run. The most important use of this new feature is to exclude/remove UI resources, like
pgAdmin,kafka-ui, etc.. that have no use during test runs.Related Issue
N/A
Type of Change
Checklist
Required
TUnit-Specific Requirements
TUnit.Core.SourceGenerator)TUnit.Engine)TUnit.Core.SourceGenerator.Testsand/orTUnit.PublicAPItests.received.txtfiles and accepted them as.verified.txt.verified.txtfiles[DynamicallyAccessedMembers]annotationsdotnet publish -p:PublishAot=trueTesting
dotnet test)Additional Notes
Note: Behavior already working on concrete projects.