Link resources in more cases #8425
Conversation
- Handle more methods - Handle more expressions - Added tests
github-actions
bot
added
the
area-app-model
There was a problem hiding this comment.
Pull Request Overview
This PR extends resource linking by adding a new method WithResourceRelationship and incorporating resource relationship assertions in tests, ensuring that resource relationships (specifically "Reference" type) are correctly applied and verified. Key changes include:
- Introducing the WithResourceRelationship extension method (with several overloads) for better resource relationship handling.
- Adding assertions in multiple test files to verify that resource relationships are set correctly.
- Updating existing resource-related methods to use the new linking method.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/Aspire.Hosting.Tests/WithReferenceTests.cs | Added assertions to verify resource relationship annotations for reference endpoints. |
| tests/Aspire.Hosting.Tests/WithEnvironmentTests.cs | Inserted assertions to ensure environment variables include proper resource relationship annotations. |
| tests/Aspire.Hosting.Tests/ProjectResourceTests.cs | Added check confirming that relationships are not processed via callbacks. |
| tests/Aspire.Hosting.Tests/ExecutableResourceTests.cs | Added assertions to validate resource relationships for executables. |
| tests/Aspire.Hosting.Containers.Tests/ContainerResourceTests.cs | Updated test to check for absence of resource relationship annotations in certain cases. |
| tests/Aspire.Hosting.Azure.Tests/AzureBicepProvisionerTests.cs | Extended test assertions to verify proper resource relationships in Azure Bicep provisioning. |
| src/Aspire.Hosting/ResourceBuilderExtensions.cs | Replaced calls to WithRelationship with the new WithResourceRelationship and added supportive overloads. |
| src/Aspire.Hosting/ConnectionStringBuilderExtensions.cs | Modified chaining to include resource relationship linking for connection strings. |
| src/Aspire.Hosting.Azure/AzureBicepResourceExtensions.cs | Added resource relationship linking in methods handling environment variables and parameters. |
Comments suppressed due to low confidence (2)
src/Aspire.Hosting/ResourceBuilderExtensions.cs:1689
- [nitpick] The relationship type 'Reference' is hardcoded in the WithResourceRelationship methods. If additional relationship types are anticipated in the future, consider making the relationship type configurable rather than fixed.
return builder.WithAnnotation(new ResourceRelationshipAnnotation(resource, KnownRelationshipTypes.Reference));
src/Aspire.Hosting/ResourceBuilderExtensions.cs:1660
- [nitpick] There appears to be no direct test coverage for the new WithResourceRelationship extension methods handling null or invalid inputs. Consider adding unit tests to ensure these methods throw the expected exceptions when provided with invalid arguments.
public static IResourceBuilder<T> WithRelationship<T>(this IResourceBuilder<T> builder, IResource resource, string type)
| arg => Assert.Equal("http://localhost:1234", arg)); | ||
|
|
||
| // We don't yet process relationships set via the callbacks | ||
| Assert.False(project.Resource.TryGetAnnotationsOfType<ResourceRelationshipAnnotation>(out var relationships)); |
There was a problem hiding this comment.
This is a case we're not handling #8426
There was a problem hiding this comment.
How might we solve this in the future. I think it makes sense to immediately add resource relationships when you can like this, but being able to catch these late discovered relationships is also useful.
So do we put the logic in two places.
If we try to put the logic in one place like in DCP integration, then any non-DCP resources won't be able to play (anything that evaluates expressions would need to the logic to add these relationships).
There was a problem hiding this comment.
I started that way but decided to back out for 9.2. We’d do it in the application orchestrator before start.
…rove clarity in WalkAndLinkResourceReferences method; add test for same resource in single expression
| /// <param name="builder">The resource builder.</param> | ||
| /// <param name="resource">The resource that the relationship is to.</param> | ||
| /// <returns>A resource builder.</returns> | ||
| public static IResourceBuilder<T> WithResourceRelationship<T>( |
There was a problem hiding this comment.
What is the difference between this and WithRelationship? Just hardcoding a relationship type of Reference?
ResourceRelationship is a generic name that doesn't mean anything. Aren't all relationships between resources resource relationships?
Shouldn't this be called WithReferenceRelationship?
There was a problem hiding this comment.
I'm not hung up on the naming, and yes it's to avoid the magic "Reference" string.
Description
Added resource linking in more methods and handle more cases (like ReferenceExpression's). Exposed a new method
WithResourceRelationshipto avoid leaking the name "Resource" as a known relationship type. Added lots of tests to assert the current behavior.Fixes #8345
Checklist
<remarks />and<code />elements on your triple slash comments?breaking-changetemplate):doc-ideatemplate):