Skip to content
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

Make unnamed volume names more unique #5779

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Make unnamed volume names more unique #5779

wants to merge 1 commit into from

Conversation

davidfowl
Copy link
Member

@davidfowl davidfowl commented Sep 19, 2024 •
edited by dotnet-policy-service bot
Loading

Description

  • Follow a scheme similar to what we do for persistent containers. Use the first 10 characters of the SHA256 of the app host's physical path as the volume name prefix.
  • This is a breaking change and the only workaround is the manually specify the volume name.

Fixes #5413

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

@davidfowl
Make unnamed volume names more unique
e469509 
- Follow a scheme similar to what we do for persistent containers. Use the first 10 characters of the SHA256 of the app host's physical path as the volume name prefix.
- This is a breaking change and the only workaround is the manually specify the volume name.

Fixes #5413
@davidfowl davidfowl added this to the 9.0 milestone Sep 19, 2024
@davidfowl davidfowl added area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. labels Sep 19, 2024
eerhardt
eerhardt reviewed Sep 19, 2024
View reviewed changes
src/Shared/VolumeNameGenerator.cs
// Create volume name like "{sha256}-postgres-data"

// Compute a short hash of the content root path to differentiate between multiple AppHost projects with similar volume names
var applicationName = builder.ApplicationBuilder.Configuration["AppHost:Sha256"]![..10].ToLowerInvariant();
Copy link
Member

Choose a reason for hiding this comment

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

As a user, how am I supposed to know which volume maps to which app in Docker Desktop?

Copy link
Member Author

Choose a reason for hiding this comment

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

You don't know the SHA256 off the top of your head?!? I was waiting for feedback to say we should keep the assembly name as part of the prefix. I think instead of what it does today around dropping the name if the characters are invalid, we should instead remove those characters.

Copy link
Member

Choose a reason for hiding this comment

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

Is there a length limit? Maybe we put the 5-10 character SHA at the end of the volume name?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the limit is OS specific (these end up being paths). I also think we should lowercase the name.

Copy link
Member

Choose a reason for hiding this comment

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

I also think we should lowercase the name.

I don't necessarily object to that. I just want to be able to understand what each volume is used for.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think we should keep the application name in here and append the SHA to disambiguate.

Copy link
Member

Choose a reason for hiding this comment

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

Also you'll need to update the doc-comment to reflect whatever you end up making the format.

Copy link
Member Author

Choose a reason for hiding this comment

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

Doc comments on internals APIs 👀

Copy link
Member

Choose a reason for hiding this comment

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

Then delete it 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eerhardt eerhardt eerhardt left review comments

@DamianEdwards DamianEdwards Awaiting requested review from DamianEdwards

@mitchdenny mitchdenny Awaiting requested review from mitchdenny mitchdenny is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication breaking-change Issue or PR that represents a breaking API or functional change over a prerelease.
Projects
None yet
Milestone
9.0
3 participants
@davidfowl @DamianEdwards @eerhardt