Perl ts integration#1362
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/CommunityToolkit/Aspire/main/eng/scripts/dogfood-pr.sh | bash -s -- 1362Or
iex "& { $(irm https://raw.githubusercontent.com/CommunityToolkit/Aspire/main/eng/scripts/dogfood-pr.ps1) } 1362" |
There was a problem hiding this comment.
Pull request overview
This PR extends the existing CommunityToolkit.Aspire.Hosting.Perl integration to support Aspire’s TypeScript/polyglot authoring flow by exporting Perl builder APIs for TS generation, adding a TypeScript AppHost example + tests, and enhancing the shared TypeScript AppHost validation tooling to support more validation scenarios (configured package mappings, optional startup, and HTTP probing).
Changes:
- Add
AspireExportattributes to Perl builder extension methods so they can be consumed from generated TypeScript AppHosts. - Add a Perl TypeScript AppHost example (
cpanm-api-integration) plus new tests that validate startup and endpoint behavior via HTTP probes. - Enhance the TypeScript AppHost validation harness (C# + PowerShell) with
-UseConfiguredPackages,-SkipStart, optional AppHost path override, and post-start HTTP probing.
Reviewed changes
Copilot reviewed 16 out of 18 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/CommunityToolkit.Aspire.Testing/TypeScriptAppHostTest.cs | Extends shared TS AppHost test runner with configured-packages, skip-start, HTTP probe, and custom path options. |
| tests/CommunityToolkit.Aspire.Hosting.Perl.Tests/WithCertificateTrustTests.cs | Adds a propagation test for Perl certificate trust when using project dependency installers. |
| tests/CommunityToolkit.Aspire.Hosting.Perl.Tests/TypeScriptAppHostTests.cs | New Perl TypeScript AppHost integration tests using HTTP probes. |
| tests/CommunityToolkit.Aspire.Hosting.Perl.Tests/AddPerlExecutableTests.cs | Adds coverage ensuring AddPerlExecutable doesn’t register interpreter-related required commands. |
| src/CommunityToolkit.Aspire.Hosting.Perl/PerlAppResourceBuilderExtensions.PackageManager.cs | Adds AspireExport for package manager-related fluent APIs. |
| src/CommunityToolkit.Aspire.Hosting.Perl/PerlAppResourceBuilderExtensions.cs | Adds AspireExport for AddPerl*/Perl fluent APIs; adjusts required-command registration for executable entrypoints. |
| src/CommunityToolkit.Aspire.Hosting.Perl/CommunityToolkit.Aspire.Hosting.Perl.csproj | Suppresses ASPIREATS001 for using experimental AspireExport. |
| examples/perl/cpanm-api-integration/scripts/cpanfile | Adds a cpanfile for project dependency installation. |
| examples/perl/cpanm-api-integration/scripts/API.pl | Adds /cert-env endpoint for validating cert env propagation. |
| examples/perl/cpanm-api-integration/README.md | Removes the example README. |
| examples/perl/cpanm-api-integration/CpanmApiIntegration.AppHost.TypeScript/tsconfig.json | New TS AppHost TypeScript configuration. |
| examples/perl/cpanm-api-integration/CpanmApiIntegration.AppHost.TypeScript/package.json | New TS AppHost package manifest (dev tooling + deps). |
| examples/perl/cpanm-api-integration/CpanmApiIntegration.AppHost.TypeScript/package-lock.json | Lockfile for deterministic installs in CI. |
| examples/perl/cpanm-api-integration/CpanmApiIntegration.AppHost.TypeScript/eslint.config.mjs | Adds lint configuration for the TS AppHost. |
| examples/perl/cpanm-api-integration/CpanmApiIntegration.AppHost.TypeScript/aspire.config.json | Adds TypeScript AppHost config and package-to-project mapping for local integration. |
| examples/perl/cpanm-api-integration/CpanmApiIntegration.AppHost.TypeScript/apphost.ts | New TS AppHost using the exported Perl APIs and cert trust configuration. |
| eng/testing/validate-typescript-apphost.ps1 | Enhances validation script to support configured packages, skip-start, and HTTP probing. |
| .gitignore | Ignores generated .aspire/ artifacts under TS AppHost directories. |
Files not reviewed (1)
- examples/perl/cpanm-api-integration/CpanmApiIntegration.AppHost.TypeScript/package-lock.json: Language not supported
aaronpowell
left a comment
There was a problem hiding this comment.
I have some issues with this PR:
- Are we currently not setting up the appropriate Perl toolchain before the tests using the C# AppHost? If we aren't, then I'd suggest that we're lacking test coverage that should be addressed separately
- I'm not sure I understand the motivation behind the overhaul of the PowerShell script for running the TypeScript AppHost tests - are there specific things in the way that the current script operations that results in gaps in what's needed for the Perl integration to run in TypeScript, or are you trying to add resiliency (or similar)? If it's the latter, let's split that into a separate PR so that we can start having more focused PRs
- There are some other tests that aren't TypeScript related - again let's try to split them into separate PRs rather than have PRs that are addressing multiple things at once - that makes them take more time to review
I do not think this suggests we're lacking test coverage, the unit tests cover this behavior. I originally submitted 6 examples to integration test so I could integration test everything. We pared that back to one example so I chose to show a simple C# apphost example with CPANM as the package manager. I thought it might be good - since we need to put a typescript apphost in to an integration test anyways - to diversify what we're integration testing since I didn't overload my one example with every single feature available (competing package managers like Carton, for example). I have converted the Typescript example/integration test to be a carbon copy of the C# apphost which reduces the need to make the test runner have Carton installed. Counter Question: It seems like I should probably over time expand my integration test to make the singular integration test cover every behavior?
This was actually a rabbit hole I went down that I believe was tied to the Typescript aspire.config.json having a "channel: stable" in it that prevented it from pulling the polyglot.local package from local. I have reverted it.
I have pulled out the tests that weren't related to the narrow Typescript changes. In general I am hearing that the tighter the PR the easier the review, I will consider that as I go forward. For the ps1 change, it was to put a bandaid over something screwy with my aspire.config.json for my Typescript apphost. I think what happened was - the first time it runs, if it has "Channel: stable" it can't get the polyglot.local package, but I removed the channel stable as a test and it worked, then put it back and it worked... again. I think because it is now cached so the restore is fine. I will want more time to revisit as I think there's still something to tease out there and it's very late so I'm going to revisit another night. |
Will expand more before PR.
…stion about if these validate-typescript-apphost.ps1 changes are required. Will interrogate later.
The offense is that "channel: stable" is blocking the polyglot.local packages from being picked up _UNLESS_ you have already built it and it's in your cache. This sent me down a rabbit hole that made me work around it with the powershell tweaks.
…ith the current state of .mts Typescript implementations. I am less confident about stripping down the AspireExport() and removing the description, but I believe Seb did this as part of PR 1370.
f8a5165 to
1578403
Compare
…ay back in. These are no longer needed simply because I stopped using "channel: stable" in the aspire.config.json.
I believe this removes the diff hit, I removed the newline after the }
|
Alright, I believe this is ready for re-review, but I will note that my rebase I messed up the typescript ps1, and had to stair step those back out. I am not above doing a new PR and removing the messy commit history here if that's something of concern. I also - because it was strictly related to typescript - went ahead and made the change to the .mts and to leverage the work of @sebastienros from #1370 Let me know if you don't like anything here and I'll make it right. |
|
I see that my one Typescript apphost test failed. It's this line causing it from Main, I believe. It's affecting at least the Java integration as well. Line 16 in 5e651f6 |
…t.TypeScript/aspire.config.json
|
@aaronpowell just checking to see if there is anything required before we get this merged in. I believe there is one failing test from here: #1362 (comment) which I believe is shared infra. Is there anything you'd like to see that I can work on to get it across the finish line? I am content if we're just waiting for the next release, for the record, I'm not necessarily in a rush. |
… with the appHostPathRelativeToRepo hook.
|
The changes to the TypeScript app host testing infrastructure looks to be done because the example project is not consistent with how others are setup. Admittedly, this should have been picked up on the original integration review, but why is it in the We really should be consistent unless there is a strongly justifiable reason in which patterns have to deviate. |
For context, the subfolder it is in is a remnant of thinking I could make several examples in the initial perl PR. You steered to have just one in the CT. I cut it back to just one, but they were all in sub folders because they were different examples. So that is the direct reason it's like it is today, I just cut the 6 examples in sub folders to one example in a sub folder. I'm happy to change this, my next series of PRs are to address just the integration tests as I realize they are lacking in comparison with other examples. Other examples test way more features than I did in this example due to the reason above, so I'm gonna expand the integration test a feature at a time to cover everything. Would you like me to get this in line in this PR or start the next PR chain with a standardization of the perl example? I assume it will be much easier to review if I create stacked PRs outside of this larger typescript one. |
|
Let's get it in line as part of this PR as then the changes to the TypeScript testing harness can be rolled back. And we don't necessarily need more examples in this repo, the goal is to give a quick overview of how an integration works from this repo, but more in depth/complex demos can be external |
I'll make that change later this week and request review after. Thanks! |
**Closes #1361 **
Added typescript support by attempting to follow the guide here https://aspire.dev/extensibility/multi-language-integration-authoring/
PR Checklist
Other information
Open to feedback.