Perl Hosting Integration - Unique Resource Installer Names#1286
Open
Omnideth wants to merge 1 commit intoCommunityToolkit:mainfrom
Open
Perl Hosting Integration - Unique Resource Installer Names#1286Omnideth wants to merge 1 commit intoCommunityToolkit:mainfrom
Omnideth wants to merge 1 commit intoCommunityToolkit:mainfrom
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.
Contributor
|
🚀 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" |
Contributor
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,
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.