exec: Provide a way to cleanup the temporary dir

[rburchell]

ioutil.TempDir passes the responsibility of cleanup onto us. We weren't
doing this, which leads to a temp dir that is full of runfile- temp
entries.

Thus, introduce a way to cleanup after ourselves.
This isn't exactly a problem, as most modern distributions
automatically clear stale entries out of /tmp, but it looks rather messy.

[TekWizely]

Hey again - This is great!

But I'm not sold on the log line.

Remind me, will the log go to STDERR or STDOUT ? If STDERR it might be okay.

I'm inclined to leave the line commented for now (so we don't lose it - or maybe a // TODO log? ), and defer its usage to some point where we have a verbose option.

Also, not sure if you saw the logic on line 28:

if config.ShowScriptFiles {
	fmt.Fprintln(config.ErrOut, tmpFile.Name())
} else {
	defer os.Remove(tmpFile.Name()) // clean up
}

So, hopefully the directory cleanup is just removing an empty directory.

I have the ShowScriptFiles check to skip the cleanup so i can debug the generated scripts.

Let's also add a check in your new function to skip cleanup when set.

I'll likely create an issue after this is merged to officially support some kind of --debug or --keep-scripts option to formalize the idea of keeping the scripts around.

-TW

[rburchell]

But I'm not sold on the log line.

Yeah, I agree. It'll go to stderr, but even then, I don't really like logging from an intermediate tool like this. The reason I don't really like it is that someone may be running something else that prints to stderr, and expecting it to log everything in a particular format, so this may break for them.

Having said that, on the other hand, just silently dropping it isn't great either which is why I did decide to print it eventually. I guess - longer term - maybe run itself could log to file, or something. I can just drop it for now though, I think that's easiest.

Also, not sure if you saw the logic on line 28:

Huh, I did not, no. If you agree, I can look at merging that to here, since I guess then it'll all be removed in one go... I think that makes sense?

So, hopefully the directory cleanup is just removing an empty directory.

Yep, that's correct.

I'll likely create an issue after this is merged to officially support some kind of --debug or --keep-scripts

Sure, that makes sense.

[TekWizely]

If you agree, I can look at merging that to here

Yeah that seems like a good idea.

It would introduce slightly different behavior in that, if an aborting error happened outside fo the exec method, the file(s) could be left behind uncleaned, and not just the (likely empty) directory.

I don't mind that however, and think its more efficient overall than individual defers to remove single file(s).

So the check/defer at line 28 would be removed and you'd add the keep file check into your new CleanupTemporaryDir method.

I think a comment when creating the temp file would be useful as well:

// See CleanupTemporaryDir for temp file cleanup
tmpFile, err := tempFile(fmt.Sprintf("%s-%s-*.sh", prefix, shell))

[TekWizely]

Going to merge this - The changes that I want to make are small and I can make them in post.

Thanks for the PR - Definitely like this approach over my single-file defers !

[TekWizely]

Hi @rburchell !

So with this and #38 merged, do you have any other enhancements in the pipeline?

I want to get a release created with the recent feature additions, but if you've got a few more in mind, I can hold off a bit to get them in.

Thanks for the contributions and lemme know!

-TW

[rburchell]

No, this was all I had for the moment :)