Skip to content

CLI publishing failure fixes. #8551

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 6 commits into from
Apr 4, 2025
Merged

CLI publishing failure fixes. #8551

merged 6 commits into from
Apr 4, 2025

Conversation

mitchdenny
Copy link
Member

@mitchdenny mitchdenny commented Apr 4, 2025 •
edited
Loading

This PR improves the publishing failure UX. There are a couple of improvments.

  1. Publisher selection is now using an async call from Spectre console so cancellation is faster.
  2. Changed the app host so that if there is a failure during publishing that the app host stops (fixes: Directly running publisher hangs on failure #8549)
  3. Removed an erroneous LogError when it should have been a debug message.
  4. Fixed TryGetContainerImageName which was also resulting in an erroneous warning/failure message when it actually worked.
  5. Reworked the publisher UX so that when publishing fails, the stage in the publishing can be more obvious. This does require some publisher cooperation to catch and log publishing activities but at a minimum we'll get an error at the top level. Fixes:

See video: #8547

aspire-publish-improved-failures.mp4

@Copilot Copilot AI review requested due to automatic review settings April 4, 2025 09:20
@github-actions github-actions bot added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Apr 4, 2025
Copilot
Copilot AI reviewed Apr 4, 2025
View reviewed changes
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.

Files not reviewed (1)
  • playground/publishers/Publishers.AppHost/qots/Dockerfile: Language not supported

@mitchdenny mitchdenny changed the title Mitchdenny/urgent-cli-fixes CLI publishing failure fixes. Apr 4, 2025
davidfowl
davidfowl reviewed Apr 4, 2025
View reviewed changes
src/Aspire.Hosting/DistributedApplicationRunner.cs
Comment on lines +71 to +74
if (!backchannelService.IsBackchannelExpected)
{
lifetime.StopApplication();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we use a finally?

@davidfowl davidfowl merged commit 9b1c4a1 into main Apr 4, 2025
174 checks passed
@davidfowl davidfowl deleted the mitchdenny/urgent-cli-fixes branch April 4, 2025 14:26
@mitchdenny
Copy link
Member Author

/backport to release/9.2

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Apr 5, 2025

Started backporting to release/9.2: https://github.com/dotnet/aspire/actions/runs/14276819232

@github-actions github-actions bot mentioned this pull request Apr 5, 2025
Merged
@github-actions github-actions bot locked and limited conversation to collaborators May 5, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

Copilot code review Copilot Copilot left review comments

@davidfowl davidfowl davidfowl approved these changes

Assignees
No one assigned
Labels
area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Directly running publisher hangs on failure
2 participants
@mitchdenny @davidfowl