Perl Hosting Integration - Unique Resource Installer Names#1286
Perl Hosting Integration - Unique Resource Installer Names#1286Omnideth wants to merge 1 commit into
Conversation
…cause I didn't enforce uniqueness on their names. The Python extension adds the parent resource name and I felt like that was an appropriate solution.
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/CommunityToolkit/Aspire/main/eng/scripts/dogfood-pr.sh | bash -s -- 1286Or
iex "& { $(irm https://raw.githubusercontent.com/CommunityToolkit/Aspire/main/eng/scripts/dogfood-pr.ps1) } 1286" |
There was a problem hiding this comment.
Pull request overview
This PR addresses Aspire resource installer name collisions in the Perl hosting integration by making per-module installer resource names unique per parent Perl app resource, and adds tests to validate the new naming/uniqueness behavior.
Changes:
- Prefix per-module installer resource names with the parent resource name to prevent collisions across multiple Perl resources.
- Update existing tests to match the new installer naming scheme.
- Add new tests covering multiple resources sharing the same packages, ensuring installer uniqueness and correct parent relationships.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/CommunityToolkit.Aspire.Hosting.Perl/PerlAppResourceBuilderExtensions.PackageManager.cs |
Changes per-module installer naming to include the parent resource name. |
tests/CommunityToolkit.Aspire.Hosting.Perl.Tests/WithPackageTests.cs |
Updates expected installer names and adds multi-resource uniqueness/parenting tests. |
tests/CommunityToolkit.Aspire.Hosting.Perl.Tests/WithLocalLibTests.cs |
Updates expected installer name to include parent resource name. |
tests/CommunityToolkit.Aspire.Hosting.Perl.Tests/InstallerIntegrationTests.cs |
Adds a Linux-only test to ensure perlbrew + shared package still yields separate installers. |
tests/CommunityToolkit.Aspire.Hosting.Perl.Tests/ApplicationModelCompositionTests.cs |
Adds a composition test ensuring shared packages across resources build successfully with unique installer names. |
Comments suppressed due to low confidence (1)
src/CommunityToolkit.Aspire.Hosting.Perl/PerlAppResourceBuilderExtensions.PackageManager.cs:397
- AddPackageInstaller uses the unsanitized
installerName(which can contain:from module names likeOpenTelemetry::SDK) when callingTryCreateResourceBuilder, but the actualPerlModuleInstallerResourceis created with a sanitized name (Replace(":", "8")). This breaks the “installer already exists” check for modules containing:and can lead to duplicate installer resources. Consider computing a single sanitized installer resource name (e.g., viaSanitizeInstallerResourceName) and using it consistently for both the lookup and the resource constructor.
var installerName = $"{resource.Resource.Name}-{packageName}-installer";
resource.ApplicationBuilder.TryCreateResourceBuilder<PerlModuleInstallerResource>(
installerName, out var existingResource);
if (existingResource is not null)
{
// Installer already exists for this package
return;
}
var installer = new PerlModuleInstallerResource(
installerName.Replace(":", "8"), // replace colons with '8' for the installer resource name (Aspire resource names cannot contain ':')
packageName,
|
We have noticed this PR has not been updated within 21 days. If there is no action on this PR in the next 14 days, we will automatically close it. You can use |
I didn't enforce uniqueness on their names. The Python extension adds the parent resource name and I felt like that was an appropriate solution.
PR Checklist
Closes #1285
Basically just adding the parent name to installer resources. Nothing fancy. A few tests to validate the installer count and names expected for various installer types. Validated in the example project for perl as well it now shows the parent name on the mojolicious installer.