Skip to content

Show neutral icon for container exiting with status code 0 - #8477

Merged
Adam Ratzman (adamint) merged 4 commits into
microsoft:mainfrom
adamint:dev/adamint/exit-code-0-show-success
Apr 8, 2025
Merged

Show neutral icon for container exiting with status code 0#8477
Adam Ratzman (adamint) merged 4 commits into
microsoft:mainfrom
adamint:dev/adamint/exit-code-0-show-success

Conversation

@adamint

@adamintAdam Ratzman (adamint) commented Apr 1, 2025

Copy link
Copy Markdown
Member

Description

Fixes#6785

image

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?

CopilotAI review requested due to automatic review settings April 1, 2025 19:56
CopilotAI reviewed Apr 1, 2025
View reviewed changes

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request introduces a change to display a neutral icon (RecordStop) for containers that exit with status code 0 in addition to finished containers. The changes include updating the inline test data to verify the new behavior, modifying the icon selection logic to support exited state with exit code 0, and adding a new helper method (IsExitedState) to support the updated condition.

Reviewed Changes

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

FileDescription
tests/Aspire.Dashboard.Tests/Model/ResourceStateViewModelTests.csAdds inline test data for exited state with exit code 0 verifying the neutral icon and color.
src/Aspire.Dashboard/Model/ResourceStateViewModel.csUpdates the conditional logic to include exited state when a valid exit code is present.
src/Aspire.Dashboard/Extensions/ResourceViewModelExtensions.csIntroduces the IsExitedState method to clearly identify exited state resources.

Comment threadsrc/Aspire.Dashboard/Model/ResourceStateViewModel.cs Outdated
JamesNK
James Newton-King (JamesNK) reviewed Apr 2, 2025
View reviewed changes
Comment threadsrc/Aspire.Dashboard/Model/ResourceStateViewModel.cs Outdated
color = Color.Error;
}
else if (resource.IsFinishedState())
else if (resource.TryGetExitCode(out _) && (resource.IsFinishedState() || resource.IsExitedState()))

@JamesNKJames Newton-King (JamesNK)Apr 2, 2025•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is TryGetExitCode here for? Are you making it so there must be a known 0 exit code and if there isn't then show warning?

The concept of exit codes is specific to executable/containers. Other resource types could go into a finished/exited state without specifying one and then show a warning.

Please test these changes with resource types other than executables/containers.

@JamesNKJames Newton-King (JamesNK)Apr 2, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For example,

image

@davidfowl

David Fowler (davidfowl) commented Apr 2, 2025

Copy link
Copy Markdown
Collaborator

I think we should unify exited and finished. I'm not sure why we have both.

@adamint

Adam Ratzman (adamint) commented Apr 3, 2025

Copy link
Copy Markdown
MemberAuthor

I think we should unify exited and finished. I'm not sure why we have both.

Mitch Denny (@mitchdenny)Karol Zadora-Przylecki (@karolz-ms) ?

@karolz-ms

Karol Zadora-Przylecki (karolz-ms) commented Apr 3, 2025

Copy link
Copy Markdown
Contributor

In DCP Containers are Exited, and Executables are Finished at the end. It is just the way the world is 🤷‍♂️ 😄

Executables can also be Terminated, but that is related to replica sets which Aspire is not using anymore. And I have a bug to fix where Executables managed by an IDE go into Terminated state at the end; they should be Finished instead. It was supposed to be fixed in 9.2 but I ran out of time.

@JamesNK

James Newton-King (JamesNK) commented Apr 3, 2025

Copy link
Copy Markdown
Member

I'm ok with making them consistent. Exited would be the logic choice for use for both. And it lines up with exit codes.

@adamint
Adam Ratzman (adamint) merged commit 3781c4a into microsoft:mainApr 8, 2025
Ankit Jain (radical) pushed a commit that referenced this pull request Apr 9, 2025
@adamint@radical
Show neutral icon for container exiting with status code 0 (#8477)
5f89e7d
* Show neutral icon for container exiting with status code 0
* Show info/stop icons for all exited resources with status code 0
* Add finished test for custom resource, remove exit code expectation
* update tests
Youssef Fahmy (Youssef1313) added a commit that referenced this pull request Apr 9, 2025
@Youssef1313@radical
Dan Moseley@mitchdenny@davidfowl@eerhardt@joperezr@DamianEdwards@adamint
Migrate from VSTest to Microsoft.Testing.Platform (#8498)
ac098f7
* Migrate from VSTest to Microsoft.Testing.Platform
* Address TODO
* Cleanup stuff around runsettings
* Add comment
* Progress
* Remove locale
* Always show live output
* Restore test session timeout
* Fix timeout
* Fix typo
* Fix duplicate commnand-line options
* Fix Linux
* Fix for Linux
* Fix
* Fix
* Filter failing
* Fix
* ignore exit code 8
* Fix extra dot
* Ignore exit code
* Fix duplicate --ignore-exit-code
* Fix playground tests on helix
* Move to RepoTesting.props
* Fix results dir
* Fix
* Progress
* Fix duplicate command-line option
* Missing report-trx
* Run executable
* Address review comments
* Revert "Run executable"
This reverts commit d10e168.
* dotnet dll
* Introduce version selector for Aspire templates (#8625)
* Introduce version selecto to aspire new
* Update src/Aspire.Cli/Commands/NewCommand.cs
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* Update src/Aspire.Cli/Commands/NewCommand.cs
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
---------
Co-authored-by: David Fowler <davidfowl@gmail.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* Error handling for GetCapabilitiesAsync connection issues (#8614)
* Error out when GetCapabiltiesAsync is missing or unexpected exception occurs.
* Update src/Aspire.Cli/DotNetCliRunner.cs
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
---------
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* Obsolete AddAzureContainerAppsInfrastructure (#8639)
* Obsolete AddAzureContainerAppsInfrastructure
This method is no longer meant to be used. Instead developers should be calling AddAzureContainerAppEnvironment.
Covert the tests using this API to the new API
* Fix volume output naming issue
We weren't discriminating between volumes and bindmounts, which caused a cache collision.
* Switch the prefix at the beginning
* Add error handling for package updates in workflow (#8390)
* Filter out tracing health endpoints in service defaults templates (#8643)
* Filter out tracing health endpoints
Fixes#8580
* Fix typo
* Show neutral icon for container exiting with status code 0 (#8477)
* Show neutral icon for container exiting with status code 0
* Show info/stop icons for all exited resources with status code 0
* Add finished test for custom resource, remove exit code expectation
* update tests
* [tests] Correctly mark Aspire.Dashboard.Tests.Integration.Playwright.AppBarTests with RequiresPlaywright (#8647)
* [tests] Skip generating Directory.Packages.Versions.props for Test (#8621)
.. Utility projects.
* Fix running playground tests on windows
---------
Co-authored-by: Ankit Jain <radical@gmail.com>
Co-authored-by: Dan Moseley <danmose@microsoft.com>
Co-authored-by: Mitch Denny <midenn@microsoft.com>
Co-authored-by: David Fowler <davidfowl@gmail.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
Co-authored-by: Jose Perez Rodriguez <joperezr@microsoft.com>
Co-authored-by: Damian Edwards <damian@damianedwards.com>
Co-authored-by: Adam Ratzman <adam@adamratzman.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators May 9, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

Copilot code reviewCopilot
Copilot review effort, defaults to Balanced
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@JamesNKJames Newton-King (JamesNK)JamesNK approved these changes

Assignees

No one assigned

Labels

area-dashboard

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Exe and Container reaching Exit Code 0 have different terminal states for the same result.

5 participants

@adamint@davidfowl@karolz-ms@JamesNK