Skip to content

Ensure AspireStore base path is absolute#7513

Merged
sebastienros merged 4 commits intomainfrom
sebros/storeroot
Feb 10, 2025
Merged

Ensure AspireStore base path is absolute#7513
sebastienros merged 4 commits intomainfrom
sebros/storeroot

Conversation

@sebastienros
Copy link
Copy Markdown
Contributor

Description

Add better error message when the AspireStore base path is invalid
c.f. #7511

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?

{
ArgumentNullException.ThrowIfNull(basePath);

if (!Path.IsPathRooted(basePath))
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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right. I realized when the poster explained their value was obj/. I assume in the aspire solution we rewrite it for the artifacts folder. But the default for external project is obj/ as per documentation.

Does the change seem right? I tested it with different values.

Comment thread src/Aspire.Hosting.AppHost/build/Aspire.Hosting.AppHost.in.targets Outdated
…gets

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
Comment thread src/Aspire.Hosting.AppHost/build/Aspire.Hosting.AppHost.in.targets Outdated
Copy link
Copy Markdown
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

I think this looks good.

@sebastienros
Copy link
Copy Markdown
Contributor Author

/cc @DamianEdwards in case you also need paths to be absolute in the code you did

@sebastienros sebastienros merged commit 4bf0c65 into main Feb 10, 2025
@sebastienros sebastienros deleted the sebros/storeroot branch February 10, 2025 20:18
@github-actions github-actions Bot added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Mar 10, 2025
@github-actions github-actions Bot locked and limited conversation to collaborators Apr 9, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants