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

Remove unnecessary subfolders from the session temp folder #615

Merged
merged 4 commits into from
Nov 1, 2023

Conversation

danegsta
Copy link
Member

With the new randomized session folder per DCP run, the additional aspire/session/<pid> subfolders aren't necessary. This simplifies the folder structure and should keep us from leaving folders behind in temp after a run (assuming normal exit conditions). In the off chance that the session folder wasn't created ahead of time (it should be with the changes in #607), we'll also apply the proper 0700 permissions on Unix just in case.

@danegsta danegsta requested a review from karolz-ms October 31, 2023 22:37
@@ -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.

Suggested change
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
if (OperatingSystem.IsWindows())

@davidfowl
Copy link
Member

We're backporting all of these changes right?

@davidfowl
Copy link
Member

Test hang?

@davidfowl
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@danegsta danegsta merged commit 68175cc into main Nov 1, 2023
4 checks passed
@danegsta danegsta deleted the danegsta/refactorSessionFolder branch November 1, 2023 16:34
@danegsta
Copy link
Member Author

danegsta commented Nov 1, 2023

/backport to release/8.0-preview1

Copy link
Contributor

github-actions bot commented Nov 1, 2023

Started backporting to release/8.0-preview1: https://github.com/dotnet/aspire/actions/runs/6722423794

Copy link
Contributor

github-actions bot commented Nov 1, 2023

@danegsta an error occurred while backporting to release/8.0-preview1, please check the run log for details!

Error: @danegsta is not a repo collaborator, backporting is not allowed. If you're a collaborator please make sure your dotnet team membership visibility is set to Public on https://github.com/orgs/dotnet/people?query=danegsta

@danegsta
Copy link
Member Author

danegsta commented Nov 1, 2023

/backport to release/8.0-preview1

Copy link
Contributor

github-actions bot commented Nov 1, 2023

Started backporting to release/8.0-preview1: https://github.com/dotnet/aspire/actions/runs/6722435032

Copy link
Contributor

github-actions bot commented Nov 1, 2023

@danegsta backporting to release/8.0-preview1 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Patch format detection failed.
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

Copy link
Contributor

github-actions bot commented Nov 1, 2023

@danegsta an error occurred while backporting to release/8.0-preview1, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

danegsta added a commit that referenced this pull request Nov 1, 2023
* 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
danmoseley pushed a commit that referenced this pull request Nov 1, 2023
* 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
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.

5 participants