Skip to content

Show command line arguments for project resource types#6212

Merged
adamint merged 7 commits intomicrosoft:mainfrom
adamint:dev/adamint/show-project-cmd-line-args
Nov 27, 2024
Merged

Show command line arguments for project resource types#6212
adamint merged 7 commits intomicrosoft:mainfrom
adamint:dev/adamint/show-project-cmd-line-args

Conversation

@adamint
Copy link
Copy Markdown
Member

@adamint adamint commented Oct 9, 2024

Description

Shows command line arguments and executable in the source column for projects, if both are present. Additionally, improves the text visualizer view to show the entire command for both projects and executables (it currently only shows the executable name)

image
image

Fixes #5570

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?
    • Yes
      • Link to aspire-docs issue:
    • No
Microsoft Reviewers: Open in CodeFlow

@adamint adamint requested review from JamesNK and drewnoakes October 9, 2024 19:17
@JamesNK
Copy link
Copy Markdown
Member

JamesNK commented Oct 9, 2024

@davidfowl @DamianEdwards Container arguments are treated as a sensitive value in resource details. It seems inconsistent that executable and project command line arguments aren't sensitive.

@DamianEdwards
Copy link
Copy Markdown
Member

@JamesNK yeah agreed. We could be clever about it I think for projects and not hide known arguments though.

@JamesNK
Copy link
Copy Markdown
Member

JamesNK commented Oct 10, 2024

I don't think we can be clever and try to understand arguments. There are tons of different formats and there is no way for the dashboard to reliably parse them all. As far as we're concerned arguments are an opaque string.

What I think we should do:

  1. On the main grid either:
    1. Add masking to the main grid command line arguments. You would see dotnet ●●●●●●●. If there are arguments then the mask/unmask button is available to click on the column to show all values.
    2. Or simply not show command line arguments for projects and containers and expect people to view the resource details.
  2. Ensure that command line arguments are visible in resource properties like they are for containers, and they're marked as sensitive.

@DamianEdwards
Copy link
Copy Markdown
Member

I don't think we can be clever and try to understand arguments. There are tons of different formats and there is no way for the dashboard to reliably parse them all. As far as we're concerned arguments are an opaque string.

We can revisit based on how this looks once implemented, but I think specifically for project resources we can be clever enough to not hide the standard args we pass as part of orchestrating projects into an Aspire launch, e.g. --no-build, --project, -c, etc., and specifically I'm talking about the arg names, not their values.

@davidfowl
Copy link
Copy Markdown
Contributor

I think we can be clever and hide arguments with secrets. We have that knowledge and we can force people to model secrets as secret parameters to get this feature. What we do now is a bit aggressive assuming all arguments are secrets.

@davidfowl
Copy link
Copy Markdown
Contributor

For now, lets make both executable and container arguments behave the same way. Let treat both as non-secret for now and file an issue to detect secrets used in arguments and hide them if there are secrets in there.

@davidfowl
Copy link
Copy Markdown
Contributor

We can even get clever and redact the secrets 😄

@davidfowl
Copy link
Copy Markdown
Contributor

Looks like a real test failure cc @radical

@radical
Copy link
Copy Markdown
Member

radical commented Oct 10, 2024

[xUnit.net 00:07:03.16]     Aspire.Workload.Tests.BuildAndRunTemplateTests.StarterTemplateNewAndRunWithoutExplicitBuild(config: "Release") [FAIL]
[xUnit.net 00:07:03.16]       [Source for apiservice]
Expected: aspire_starter_run_Release_a10gjci0_yq5.ApiService.csproj
Actual: dotnet run --no-build --project C:\h\w\AAD0096A\t\testroot\aspire_starter_run_Release_a10gjci0_yq5\aspire_starter_run_Release_a10gjci0_yq5.ApiService\aspire_starter_run_Release_a10gjci0_yq5.ApiService.csproj -c Release --no-launch-profile

Yeah, the test needs to be updated to track changes in this PR. @adamint

@adamint
Copy link
Copy Markdown
Member Author

adamint commented Oct 14, 2024

StarterTemplateNewAndRunWithoutExplicitBuild

I keep forgetting about these workload tests 😄

# Conflicts:
#	src/Aspire.Dashboard/Components/ResourcesGridColumns/SourceColumnDisplay.razor
# Conflicts:
#	tests/Aspire.Workload.Tests/StarterTemplateRunTestsBase.cs
@adamint
Copy link
Copy Markdown
Member Author

adamint commented Oct 23, 2024

@JamesNK @davidfowl for approval

# Conflicts:
#	src/Aspire.Dashboard/Components/ResourcesGridColumns/SourceColumnDisplay.razor
#	tests/Aspire.Workload.Tests/WorkloadTestsBase.cs
@adamint
Copy link
Copy Markdown
Member Author

adamint commented Nov 21, 2024

@JamesNK for review.

@davidfowl
Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@JamesNK
Copy link
Copy Markdown
Member

JamesNK commented Nov 22, 2024

I added some args to a project in TestShop:

var catalogDbApp = builder.AddProject<Projects.CatalogDb>("catalogdbapp")
                          .WithReference(catalogDb)
                          .WithArgs(["one", "two"]);

This is what I see:

image

Is that what's indented? It seems like CatalogDb.csproj one two would be more what is expected.

Also, unit tests should be added here. GetSourceColumnValueAndTooltip could easily be moved out of the component and tested with various ResourceViewModel instances.

@davidfowl
Copy link
Copy Markdown
Contributor

Do we erase the project name when we show the args?

@adamint
Copy link
Copy Markdown
Member Author

adamint commented Nov 25, 2024

Do we erase the project name when we show the args?

Considering the project name is included in default args, yes. I have changed that now, so it will show up twice, but the executable (dotnet) does not show up in the source column when there is a project:
image
image

We could hide common arguments by default in a follow-up?

@JamesNK
Copy link
Copy Markdown
Member

JamesNK commented Nov 26, 2024

What I see now:

image

And visualizer:

image

It's not perfect, but it seems good enough. @davidfowl is this what you imagined?

@davidfowl
Copy link
Copy Markdown
Contributor

Yea, I think that's better

@adamint adamint merged commit 9dfc480 into microsoft:main Nov 27, 2024
@github-actions github-actions Bot locked and limited conversation to collaborators Dec 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The dashboard does not show command line arguments for project resources

5 participants