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.

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

[release/8.0-preview1] Remove unnecessary subfolders from the session temp folder #621

Merged
merged 1 commit into from
Nov 1, 2023

Conversation

danegsta
Copy link
Member

@danegsta danegsta commented Nov 1, 2023

Cherry-Pick #615

  • Remove extra subfolders from session temp folder path

  • Ensure the session folder is created with appropriate Unix permissions if it doesn't already exist

  • Check for Windows for proper CreateDirectory method

  • Remove extra using

* Remove extra subfolders from session temp folder path

* Ensure the session folder is created with appropriate Unix permissions if it doesn't already exist

* Check for Windows for proper CreateDirectory method

* Remove extra using
@@ -263,7 +264,14 @@ private static Socket CreateLoggingSocket(string socketPath)
string? directoryName = Path.GetDirectoryName(socketPath);
if (!string.IsNullOrEmpty(directoryName))
{
Directory.CreateDirectory(directoryName);
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
Copy link
Member

Choose a reason for hiding this comment

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

For a future PR, we can use OperatingSystem.IsWindows() here, which is the preferred way of doing this check.

Copy link
Member Author

Choose a reason for hiding this comment

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

#624 will make that change in main; I'll create a separate cherry-pick PR for it and #607

@danegsta
Copy link
Member Author

danegsta commented Nov 1, 2023

@danmoseley or @davidfowl can I get an approval from either of you so this can be merged?

Copy link
Member

@danmoseley danmoseley left a comment

Choose a reason for hiding this comment

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

this isn't a must have for preview is it? after the last change which unblocked mac.

but I'm fine taking it if you believe it's needed.

@danegsta
Copy link
Member Author

danegsta commented Nov 1, 2023

This isn't as critical as the other PR, but it does reduce the chance of hitting path limits on Mac, which was previously an issue.

@danmoseley danmoseley merged commit 0c04133 into release/8.0-preview1 Nov 1, 2023
6 checks passed
@danmoseley danmoseley deleted the danegsta/cherryPick68175cc branch November 1, 2023 20:41
joperezr added a commit that referenced this pull request Nov 14, 2023
@danmoseley danmoseley added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Nov 16, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Apr 30, 2024
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.

4 participants