Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Packages a repo-local, containerized workflow for testing Aspire PR builds and updates Azure deployment summaries to include Azure Portal links, with documentation updates for the pr-testing skill.
Changes:
- Added
eng/scripts/aspire-pr-container/(Dockerfile + entrypoint + bash/PowerShell runners) to run PR testing consistently on macOS/Linux/WSL and Windows. - Centralized Azure Portal URL/link generation into
src/Shared/AzurePortalUrls.csand updated Azure deployment summaries to include Resource Group + Deployments + per-resource portal links. - Expanded Azure deployer tests to validate summary markdown content and portal links.
Show a summary per file
| File | Description |
|---|---|
| tests/Aspire.Hosting.Azure.Tests/AzureDeployerTests.cs | Adds portal-link-focused summary assertions and resourceId parsing helper for tests. |
| src/Shared/AzurePortalUrls.cs | New shared helper for building Azure Portal URLs and markdown links. |
| src/Aspire.Hosting.Azure/AzurePortalUrls.cs | Removes duplicated AzurePortalUrls implementation in favor of shared version. |
| src/Aspire.Hosting.Azure/AzureEnvironmentResource.cs | Uses shared portal link helpers; adds deployments link to pipeline summary. |
| src/Aspire.Hosting.Azure/Aspire.Hosting.Azure.csproj | Links shared AzurePortalUrls into project; adjusts InternalsVisibleTo. |
| src/Aspire.Hosting.Azure.AppService/AzureAppServiceWebSiteResource.cs | Adds portal link next to deployed endpoint in summary/log output; refactors site/slot naming helper. |
| src/Aspire.Hosting.Azure.AppService/Aspire.Hosting.Azure.AppService.csproj | Links shared AzurePortalUrls into project. |
| src/Aspire.Hosting.Azure.AppService/AppSvcUrls.cs | New helper to build App Service portal links from planId output. |
| src/Aspire.Hosting.Azure.AppContainers/ContainerAppUrls.cs | New helper to build Container App portal links from environmentId output. |
| src/Aspire.Hosting.Azure.AppContainers/AzureContainerAppResource.cs | Adds portal link to summary/log output for container apps (including no-public-endpoints case). |
| src/Aspire.Hosting.Azure.AppContainers/Aspire.Hosting.Azure.AppContainers.csproj | Links shared AzurePortalUrls into project. |
| eng/scripts/aspire-pr-container/run-aspire-pr-container.sh | New bash host runner to build/run the PR-testing container, optionally recording via asciinema. |
| eng/scripts/aspire-pr-container/run-aspire-pr-container.ps1 | New PowerShell host runner mirroring bash runner behavior (including optional recording). |
| eng/scripts/aspire-pr-container/entrypoint.sh | Container entrypoint: runs get-aspire-cli-pr.sh with defaults and configures NuGet source to PR hive. |
| eng/scripts/aspire-pr-container/Dockerfile | New container image definition for PR testing. |
| .github/skills/pr-testing/SKILL.md | Updates guidance to prefer repo-local container runner and documents setup/recording/inspection workflows. |
Copilot's findings
- Files reviewed: 16/16 changed files
- Comments generated: 4
| fi | ||
|
|
||
| if [[ -e "$DOCKER_SOCKET_PATH" ]]; then | ||
| DOCKER_SOCKET_REALPATH="$(python3 -c 'import os, sys; print(os.path.realpath(sys.argv[1]))' "$DOCKER_SOCKET_PATH")" |
There was a problem hiding this comment.
The runner requires python3 on the host to resolve the Docker socket realpath, but python3 isn't guaranteed to be installed (common on minimal distros or some macOS setups). Replace this with a more portable approach (e.g., prefer realpath if available, else fall back to readlink -f, else use the original path) and emit a clear warning if resolution isn't possible.
Comment on lines
+116
to
+122
| if [[ -z "${GH_TOKEN:-}" && -z "${GITHUB_TOKEN:-}" ]]; then | ||
| GH_TOKEN="$(gh auth token)" | ||
| export GH_TOKEN | ||
| elif [[ -z "${GH_TOKEN:-}" ]]; then | ||
| GH_TOKEN="$GITHUB_TOKEN" | ||
| export GH_TOKEN | ||
| fi |
There was a problem hiding this comment.
When GH_TOKEN/GITHUB_TOKEN is unset, the script invokes gh auth token but doesn't check that gh is installed or that the auth call succeeds, so failures can be hard to diagnose under set -e. Add an explicit command -v gh check (and check for an empty token) with a targeted error message describing how to authenticate or provide a token.
Comment on lines
+16
to
+19
| internal static async Task<MarkdownString> GetPortalLinkAsync(AzureAppServiceEnvironmentResource computerEnv, string siteName, string? deploymentSlot, CancellationToken cancellationToken) | ||
| { | ||
| var planIdValue = await computerEnv.PlanIdOutputReference.GetValueAsync(cancellationToken).ConfigureAwait(false) | ||
| ?? throw new InvalidOperationException($"Missing app service plan id output for '{computerEnv.Name}'."); |
There was a problem hiding this comment.
The parameter name computerEnv looks like a typo (likely meant to be computeEnv or appServiceEnv). Renaming it would improve readability and reduce confusion with unrelated 'computer' terminology.
Suggested change
| internal static async Task<MarkdownString> GetPortalLinkAsync(AzureAppServiceEnvironmentResource computerEnv, string siteName, string? deploymentSlot, CancellationToken cancellationToken) | |
| { | |
| var planIdValue = await computerEnv.PlanIdOutputReference.GetValueAsync(cancellationToken).ConfigureAwait(false) | |
| ?? throw new InvalidOperationException($"Missing app service plan id output for '{computerEnv.Name}'."); | |
| internal static async Task<MarkdownString> GetPortalLinkAsync(AzureAppServiceEnvironmentResource appServiceEnv, string siteName, string? deploymentSlot, CancellationToken cancellationToken) | |
| { | |
| var planIdValue = await appServiceEnv.PlanIdOutputReference.GetValueAsync(cancellationToken).ConfigureAwait(false) | |
| ?? throw new InvalidOperationException($"Missing app service plan id output for '{appServiceEnv.Name}'."); |
Comment on lines
+1388
to
+1395
| var subscriptionId = resourceIdentifier.SubscriptionId; | ||
| var resourceGroupName = resourceIdentifier.ResourceGroupName; | ||
|
|
||
| return (subscriptionId, resourceGroupName) switch | ||
| { | ||
| ({ Length: > 0 }, { Length: > 0 }) => (subscriptionId, resourceGroupName), | ||
| _ => throw new InvalidOperationException($"Resource id '{resourceId}' does not contain both subscription and resource group segments.") | ||
| }; |
There was a problem hiding this comment.
ResourceIdentifier.SubscriptionId / .ResourceGroupName are nullable in Azure.Core APIs; assigning them to non-nullable locals can produce nullable warnings (and can be inconsistent with the other helpers added in this PR that use ?? throw). Consider switching these locals to string? and using ?? throw with a dedicated message for each missing segment, or keep the tuple-pattern approach but ensure the local types match the API nullability to avoid warnings-as-errors in test builds.
Suggested change
| var subscriptionId = resourceIdentifier.SubscriptionId; | |
| var resourceGroupName = resourceIdentifier.ResourceGroupName; | |
| return (subscriptionId, resourceGroupName) switch | |
| { | |
| ({ Length: > 0 }, { Length: > 0 }) => (subscriptionId, resourceGroupName), | |
| _ => throw new InvalidOperationException($"Resource id '{resourceId}' does not contain both subscription and resource group segments.") | |
| }; | |
| var subscriptionId = resourceIdentifier.SubscriptionId | |
| ?? throw new InvalidOperationException($"Resource id '{resourceId}' does not contain a subscription segment."); | |
| var resourceGroupName = resourceIdentifier.ResourceGroupName | |
| ?? throw new InvalidOperationException($"Resource id '{resourceId}' does not contain a resource group segment."); | |
| if (subscriptionId.Length == 0) | |
| { | |
| throw new InvalidOperationException($"Resource id '{resourceId}' contains an empty subscription segment."); | |
| } | |
| if (resourceGroupName.Length == 0) | |
| { | |
| throw new InvalidOperationException($"Resource id '{resourceId}' contains an empty resource group segment."); | |
| } | |
| return (subscriptionId, resourceGroupName); |
Add repo-local PR testing container runners for bash and PowerShell, including optional host-side asciinema recording and workspace reuse guidance. Update the PR testing skill to prefer the container runner, document Windows usage, and capture the setup, apphost, and inspection workflows used during manual PR validation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Treat the PR install script as the normal path again and leave setup --force as troubleshooting-only guidance for bundle extraction failures. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Treat bundle extraction/layout failures as failures to report rather than steps the agent should auto-repair. Keep the PR-testing skill aligned with what a normal user would actually do after installing a PR build. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
davidfowl
force-pushed
the
davidfowl/pr-testing
branch
from
ad31191 to
38eca9f
Compare
Contributor
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 16184Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 16184" |
Strip the install/NuGet automation from the container entrypoint so the runner is just an isolated environment. The dogfood command from the PR comment is used directly inside the container instead. - Entrypoint reduced to HOME fallback + exec - Runner scripts no longer pass INSTALL_PREFIX - Default command is bash when no args given - Dockerfile unchanged (Node/Python deferred) - Skill updated to use dogfood command directly Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
davidfowl
force-pushed
the
davidfowl/pr-testing
branch
from
4630fa6 to
762c583
Compare
The CLI is a self-contained native binary so we don't need the .NET SDK base image. Ubuntu 24.04 is smaller and has glibc compatibility for the native AOT binary. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
davidfowl
force-pushed
the
davidfowl/pr-testing
branch
from
762c583 to
5f3d3e9
Compare
Contributor
|
Re-running the failed jobs in the CI workflow for this pull request because 2 jobs were identified as retry-safe transient failures in the CI run attempt.
|
Contributor
|
🎬 CLI E2E Test Recordings — 71 recordings uploaded (commit View recordings
📹 Recordings uploaded automatically from CI run #24435165743 |
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.
Description
Add a repo-local container workflow for Aspire PR testing and document the tested usage in the
pr-testingskill.eng/scripts/aspire-pr-container/with bash and PowerShell runners plus the container image entrypoint.asciinemarecording and keeping the mounted workspace around for later inspection.pr-testingskill to prefer the repo-local runner, document Windows PowerShell usage, and capture the setup, AppHost, recording, and inspection guidance learned while dogfooding PR builds.This change packages the containerized PR-testing flow used during manual validation so it can be reused consistently from the repo on macOS/Linux/WSL and Windows PowerShell hosts.
No code dependencies were added. The container workflow relies on Docker and
gh, and recording additionally requiresasciinemaon the host.Validation:
aspire describevalidation scenarios for Rename describe endpoints column to URLs #16144 in the repo-local container runner.ASPIRE_PR_RECORD=1on the bash runner.bash -n eng/scripts/aspire-pr-container/run-aspire-pr-container.sh.run-aspire-pr-container.ps1was not executed in this environment becausepwshwas unavailable.Fixes # (issue)
Checklist
<remarks />and<code />elements on your triple slash comments?aspire.devissue: