fix: handle users/groups with no names in push_path

[tonyandrewmeyer]

Ned Batchelder noticed that the unit tests failed on his machine, which was because the temporary folder returned by TemporaryDirectory() belonged to a group with no name.

In theory, this could happen outside of the tests as well - someone could do a push_path() on a path that has files that have owners/groups that do not have names (e.g. because they have been removed). This is presumably quite uncommon (which makes sense: it seems unlikely that the charm container would often have a user or group removed) but it's also simple to fix, so it seems like we might as well do that.

[benhoyt]

A couple of nit suggestions and one question about the testing.

[benhoyt]

It's a bit weird that we're not actually testing that the user/group attributes are None. In fact, this test succeeds even if you comment out the patching. I guess there's not an easy way to test this, because in this code path we're not actually using the user/group attributes. We could test the logs generated, but probably not worth it.

It does seem strange that we're calling these functions on every file and then throwing away the data (_list_recursive only uses the type attributes). But I'm happy to keep it as is for now to avoid code churn.

Thoughts?

[tonyandrewmeyer]

It's a bit weird that we're not actually testing that the user/group attributes are None. In fact, this test succeeds even if you comment out the patching. I guess there's not an easy way to test this, because in this code path we're not actually using the user/group attributes.

Yes, that was the issue I had 😞. The test does fail (error) without the model.py change (as long as the patching is there), because a KeyError will be raised, but it indeed doesn't validate what the alternate value is. If you comment out the patching, then it's just testing that the regular case (whatever user/group you're defaulting to) works.

We could test the logs generated, but probably not worth it.

Agreed.

It does seem strange that we're calling these functions on every file and then throwing away the data (_list_recursive only uses the type attributes). But I'm happy to keep it as is for now to avoid code churn.

Thoughts?

I found it odd too. At first I thought it was intending to set the user/group on the workload container (which seems dubious using the ID), but noticed that indeed the majority of the data is thrown away.

However, there's bleeding over into harness, because the test pebble's list_files uses the same method so in tests people get back all the info. (I could actually put it in a test there that checks that the names are None).

It does seem like it would be cleaner if _TestingPebbleClient had the _build_fileinfo that Container has in the current PR (or one that started with Container's and built on it like the current PR), and Container had something like:

    @staticmethod
    def _build_fileinfo(path: Union[str, Path]) -> pebble.FileInfo:
        """Constructs a FileInfo object by stat'ing a local path."""
        path = Path(path)
        if path.is_symlink():
            ftype = pebble.FileType.SYMLINK
        elif path.is_dir():
            ftype = pebble.FileType.DIRECTORY
        elif path.is_file():
            ftype = pebble.FileType.FILE
        else:
            ftype = pebble.FileType.UNKNOWN

        return pebble.FileInfo(
            path=str(path),
            name=path.name,
            type=ftype)

... except that FileInfo has last_modified and permissions as required fields. A stat is pretty cheap, but it's also being completely wasted here.

Container's _build_fileinfo doesn't have to return a FileInfo (although the name would need to change if it didn't). I guess it could return a TypedDict private to model.py that only had the path and type.

This would extend the value of this PR to actually making a (presumably imperceptible) performance improvement. But it also significantly increases the number of lines touched for what was originally a (I assume) fairly obscure bug.

[benhoyt]

Thanks for the write-up. Okay, I think let's leave this PR as just the bug-fix: it's definitely an improvement over what we have now. We can always consider the simplification/refactor later separately for performance reasons.

[benhoyt]

Looks good to me now, though linting is failing.

[tonyandrewmeyer]

Looks good to me now, though linting is failing.

Fixed now. Do you have a suggestion for a second reviewer?

[benhoyt]

I'm fine without a second reviewer on this small bug fix. I'll merge as soon as the tests are passing.