8000 PublishingActivityProgressReporter fixes by mitchdenny · Pull Request #8571 · dotnet/aspire · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

PublishingActivityProgressReporter fixes #8571

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 7, 2025
Merged

PublishingActivityProgressReporter fixes #8571

merged 6 commits into from
Apr 7, 2025

Conversation

mitchdenny
Copy link
Member
@mitchdenny mitchdenny commented Apr 6, 2025 •
edited
Loading

Fixes: #8570

This PR fixes a race condition that existed with the PublishingActivityProgressReporter API. The issue was that when someone used the old API to update the status/create the status of an issue it was possible for another piece of code to mutate the state of the publishing activity.

This in turn resulted in internal logic in the AppHostRpcTarget detecting that a publishing activity was completed and yield returning the activity stream without actually transmitting the completed activity.

This resulted in the CLI interpretting this as a failure state (even though the apphost returned a zero exit code).

The fix was to rework the API for the PublishingActivityProgressReporter so that it is similar to what we do with the ResourceNotificationService. Now we have an API where we create a new status object based on the old one.

Here is an example of the new usage:

// Create activity
var publishingActivity = await activityReporter.CreateActivityAsync(
  "publishing-artifacts",
  $"Executing publisher {executionContext.PublisherName}",
  isPrimary: true,
  stoppingToken).ConfigureAwait(false);

// Update activity status.
await activityReporter.UpdateActivityStatusAsync(
  publishingActivity,
  (status) => status with { IsComplete = true },
  stoppingToken).ConfigureAwait(false);

Here is a video of Kubernetes publisher working where this issue was first noticed:

aspire-activityreporter-fixes.mp4

@Copilot Copilot AI review requested due to automatic review settings April 6, 2025 01:47
@mitchdenny mitchdenny linked an issue Apr 6, 2025 that may be closed by this pull request
aspire publish with kubernetes publisher fails the first time #8570
Closed
@github-actions github-actions bot added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Apr 6, 2025
Copilot
Copilot AI reviewed Apr 6, 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 6 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

src/Aspire.Cli/Commands/PublishCommand.cs:178

  • [nitpick] The variable 'lastPublishingActivity' is assigned within the loop but never used elsewhere. Consider removing it if it is redundant, or integrate it in subsequent logic if it was intended to be used.
                (string Id, string StatusText, bool IsComplete, bool IsError)? lastPublishingActivity = null;

@mitchdenny mitchdenny requested a review from davidfowl April 6, 2025 01:50
@mitchdenny mitchdenny self-assigned this Apr 6, 2025
@mitchdenny mitchdenny added this to the 9.2 milestone Apr 6, 2025
davidfowl
davidfowl reviewed Apr 6, 2025
View reviewed changes
davidfowl
davidfowl reviewed Apr 6, 2025
View reviewed changes
@mitchdenny mitchdenny requested a review from davidfowl April 7, 2025 01:16
@mitchdenny mitchdenny merged commit 022a450 into main Apr 7, 2025
174 checks passed
@mitchdenny mitchdenny deleted the mitchdenny/fix-8570 branch April 7, 2025 01:41
@mitchdenny
Copy link
Member Author

/backport to release/9.2

@github-actions GitHub Actions
Copy link
Contributor
github-actions bot commented Apr 7, 2025

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

@github-actions github-actions bot mentioned this pull request Apr 7, 2025
Merged
@github-actions github-actions bot locked and limited conversation to collaborators May 7, 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

@mitchdenny mitchdenny

Labels
area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication area-cli
Projects
None yet
Milestone
9.2
Development

Successfully merging this pull request may close these issues.

aspire publish with kubernetes publisher fails the first time
2 participants
@mitchdenny @davidfowl
0