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

Create working dir if it does not exist #1691

Merged
merged 2 commits into from
Sep 12, 2023
Merged

Create working dir if it does not exist #1691

merged 2 commits into from
Sep 12, 2023

Conversation

rylev
Copy link
Collaborator

@rylev rylev commented Aug 14, 2023

Currently we error if the working directory does not exist already.

@rylev rylev requested a review from itowlson August 14, 2023 12:21
Copy link
Contributor

@itowlson itowlson left a comment

Choose a reason for hiding this comment

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

I can't remember what discussions we had at the time about whether implicit directory creation was good (so convenient) or bad (what if they meant ~/myapps but typoed ~/maypps) - other people may have views on this...

.path()
.canonicalize()
.context("Could not canonicalize working directory")?;
Ok(working_dir)
Copy link
Contributor

Choose a reason for hiding this comment

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

I might be missing something but this looks like it drops working_dir_holder. This needs to be kept alive until the trigger returns because it controls the deletion of temporary directories. (This is why the WorkingDirectory type exists - I needed something to sit in the main flow that had more ownership than a PathBuf.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Very good catch! I'll change that and make a comment.

@vdice vdice added this to the 1.5.0 milestone Aug 15, 2023
@vdice
Copy link
Member

vdice commented Aug 23, 2023

E2e tests should now run successfully, just need to rebase. Though, it looks like there is a failure in the Go SDK tests as well.

rylev added 2 commits August 25, 2023 18:03
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
@rylev rylev force-pushed the create-woring-dir branch from 1d68d7e to e3ab6af Compare August 25, 2023 16:04
@rylev
Copy link
Collaborator Author

rylev commented Aug 25, 2023

@itowlson the issue around the temp directory has been fixed. How do you want to make a call on whether auto creation of the working dir is the right way to go?

Copy link
Contributor

@itowlson itowlson left a comment

Choose a reason for hiding this comment

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

LGTM - thanks for adding the comment.

Re whether the behaviour is desirable or not, I can't find any record of the alleged previous discussion. I don't have strong feelings either way, so I guess either wield your maintainer powers or do a quick poll in Discord?

@mikkelhegn
Copy link
Member

LGTM

@rylev rylev merged commit 69c13c7 into main Sep 12, 2023
@rylev rylev deleted the create-woring-dir branch September 12, 2023 09:32
@rylev
Copy link
Collaborator Author

rylev commented Sep 12, 2023

Note to future selves - this was discussed by several folks including @mikkelhegn and @melissaklein24, and everyone seemed relatively happy with this small change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants