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

Use Directory.CreateTempSubdirectory #607

Merged
merged 2 commits into from
Nov 1, 2023
Merged

Use Directory.CreateTempSubdirectory #607

merged 2 commits into from
Nov 1, 2023

Conversation

eerhardt
Copy link
Member

The advantage here is that the folder gets the correct permissions on Unix.

@danegsta - who cleans up this folder?

@eerhardt eerhardt requested a review from danegsta October 31, 2023 20:47
@danegsta
Copy link
Member

The advantage here is that the folder gets the correct permissions on Unix.

@danegsta - who cleans up this folder?

The original structure for the temp assets was %TEMP%/aspire/session/<pid>, where <pid> is the PID of the executing application (usually the AppHost) in order to make it easier to track down specific assets for an aspire run, like the kubeconfig for DCP. That was causing issues with tests that might need to run multiple tests which would end up colliding on the PID of the test host.

DCP will cleanup the <pid> folder when it exits normally, but the rest of the parent folders don't have anything cleaning them up.

I'm wondering if we should consider refactoring back to %TEMP%/aspire/<pid> and remove the session folder, plus add a new property to DistributedApplicationOptions to override the %TEMP%/aspire/ path for tests, since that gives us only one orphan aspire temp folder for normal operations (and makes it easy to track down the kubeconfig for diagnostics), while letting tests set a unique value to avoid collisions.

@danegsta
Copy link
Member

If the given path doesn't exist, we specifically create it as part of creating the folder to hold the unix domain socket we use for sending system logs from DCP back to the AppHost. If the default for Directory.CreateDirectory isn't 0700 on Unix, we should probably update that call as well.

@danegsta
Copy link
Member

But now that I think about it, using a shared root aspire folder is asking for trouble on Unix systems if anyone ever runs an AppHost as a different user (or something changed the permissions on the folder), so sticking with a simple Directory.CreateTempSubdirectory() folder and removing all the extraneous aspire/session/<pid> subpath is the safest overall option.

@eerhardt eerhardt merged commit e05579c into main Nov 1, 2023
4 checks passed
@eerhardt eerhardt deleted the eerhardt-patch-1 branch November 1, 2023 02:28
@eerhardt
Copy link
Member Author

eerhardt commented Nov 1, 2023

Note another change we could have made is to add a prefix - the CreateTempSubdirectory takes an optional (string? prefix = null). So we can pass "aspire" in for the prefix and all the temp directories will have that prefix.

danegsta pushed a commit that referenced this pull request Nov 1, 2023
* Use Directory.CreateTempSubdirectory

The advantage here is that the folder gets the correct permissions on Unix.
davidfowl pushed a commit that referenced this pull request Nov 2, 2023
* Use Directory.CreateTempSubdirectory (#607)

* Use Directory.CreateTempSubdirectory

The advantage here is that the folder gets the correct permissions on Unix.

* Improvements for temp paths and OS check (#624)

---------

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
joperezr added a commit that referenced this pull request Nov 14, 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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants