feat: make Juju.temp_dir public, improve docs

[benhoyt]

This makes the Juju._temp_dir helper public, for use by tests that do scp and the like, when Juju is running as a snap.

I'm not entirely sure this is a good idea, or necessarily the API we want, so would like feedback on that.

Fixes #201.

[james-garner-canonical]

This seems like a reasonable solution to the reported issue. I've noted a few potential additions to the documentation, as well as suggested an alternative method name. The return type should probably be a Path too.

I thought about whether this should be a top-level helper function instead, but I think it will be a lot more ergonomic as a method since we need to know the binary location, so a Juju instance would likely be a required argument anyway.

[james-garner-canonical]

Probably nicer to return a pathlib.Path object instead of a str -- path-like objects can be used with the dir argument of the tempfile functions.

[james-garner-canonical]

We should make it clear that this returns the path to the conventional system or snap temporary directory. The fact that it's a cached property helps, but I do wonder if users will at first think of using it like you would with tempfile.TemporaryDirectory() ....

Maybe the initial line could end with 'typically /tmp' to make it clear via an example.

I wonder if 'The directory is created and cached on first use.' should be expanded to include:

• The directory is created on first use if it doesn't exist.
• The cached path is returned on later uses.
• This will be the same directory for all Juju instances with the same binary, and indeed across multiple runs of the program.
• We don't clean up any files created there.

[james-garner-canonical]

Suggested change

	is installed as a confined snap). See :attr:`temp_dir` for an example.
	is installed as a confined snap). See :attr:`temp_dir` for an example of how to create a
	temporary file accessible by Juju.

[james-garner-canonical]

Maybe the name default_temp_dir would make the semantics clearer, though it would mean that what we're asking users to write whenever they use tempfile methods would be a little bit longer.

[dimaqq]

If temp_dir is a pathlib.Path, I'd personally use it like this:

Suggested change

	with tempfile.NamedTemporaryFile('w+', dir=juju.temp_dir) as file:
	file.write('contents')
	file.flush()
	juju.scp(file.name, 'ubuntu/0:/path/to/destination')
	(juju.temp_dir / 'cont').write_text('contents')
	juju.scp(juju.temp_dir / 'cont', 'ubuntu/0:/path/to/destination')

Practicality over purity: if it's an integration test, there will be only a few named files.

The named temp file would be still useful in a case where Jubilant is used in production and the process is long-lived. In that case very many scp calls could be expected.

[james-garner-canonical]

Brainstorming a little bit, I agree that this is a nice UX -- probably the way we'd normally do it in tests is using pytest's tmp_path fixture, which takes care of cleanup when the fixture exits. But of course that specific fixture won't work with snap Juju. I'm wondering if we could provide cleanup functionality in a similar way though. For example, Juju.temp_dir could create and return a subdir of the current temp location and record created directories, and the juju fixture could clean them up on exiting.

[dimaqq]

Looks good overall.
Let's settle on the API first.

[tonyandrewmeyer]

It seems like (at least with the methods we have now), the use-cases for this are:

• deploying or refreshing a charm (the charm location)
• scp (the source or destination)
• maybe ssh (for an "-i /tmp/private.key" type thing)

I wonder if it would be better to solve those cases (particularly scp, possibly deploy and refresh, and if requested, ssh) rather than provide a mechanism for the tests to solve it. For example, Jubilant could check whether Juju would be able to access the provided file and if not copy it somewhere that it could.

Perhaps this is too "magic" or high-level for Jubilant, but it seems like it would be nicer if the user didn't need to know about snap confinement or that a juju.temp_dir was necessary to pass to TemporaryDirectory.

[benhoyt]

@tonyandrewmeyer That's a good idea. I think it should be doable and not too hard:

• For deploy, if charm starts with . or /, then use a NamedTemporaryFile and shutil.copy it to temp.name first
• For refresh, if path is provided, do similar to the above
• For scp, if source is a local path (no : in it), copy it to a named temp first; otherwise if destination is a local path (no : in it), do the scp to the temp path, then copy it from there after

These would only be done if Juju was installed as a snap (Juju._juju_is_snap).

Does that seems reasonable? I think deploy and refresh are pretty obvious. I'm not totally sure about the ':' not in path idea for scp though.

I have an untested PoC of this idea here.

[james-garner-canonical]

It would probably be nice if pathlib.Path arguments always got the local file treatment too.

---

On that front, I notice that we just str them internally, which means that relative paths are a bit useless for deploy and refresh. Currently juju.deploy(Path('my.charm')) will presumably fail the way juju deploy my.charm does when you don't write juju deploy ./my.charm. But Path('./my.charm') will fail too, since that also becomes 'my.charm' when strung.

Could be made nicer with something like this:

if isinstance(charm, Path):
     if charm.is_absolute():
        charm = str(charm)
    else:
        charm = f'./{charm}'