`remove_path` method behaviour in `pebble`

[Abuelodelanada]

The remove_path method in pebble has an optional argument recursive: bool = False:

def remove_path(self, path: str, *, recursive: bool = False):

Let's say that you need to delete a file from a container's directory. But it's possible that the file you trying to delete it is not yet in the directory.

If you have:

container.remove_path(path)

You will get a PathError exception since the file does not exist. This makes sense, it is behaviourally similar to rm <file>:

➜  rm pepe.txt
rm: cannot remove 'pepe.txt': No such file or directory

But there is no explicit option that behaves similar to rm -f <file> (do nothing if the files is non-existent):

➜  rm -f pepe.txt
➜

Anyway you can achieve that behaviour by using recursive=True

container.remove_path(path, recursive=True)

recursive=True behaves in 2 ways:

• If path is a directory recursively deletes it and everything under it.
• If the path is a file, delete the file and do nothing if the file is non-existent. Behaviourally similar to rm -rf <file|dir>

Using recursive=True when trying to delete a file and do nothing if file is non-existent (rm -f <file>) is a little bit confusing.

Shouldn't we try to mimic the rm behaviour where we have: -r for recursive and -f for "ignore nonexistent files and arguments, never prompt"?

[pengale]

Thank you for filing this!

[balbirthomas]

By equating the behaviour of rm and remove_path @Abuelodelanada does make a convincing case why a -f would be useful, in the sense that it will unsurprising to the developer. However given the fact that the objective of remove_path is to ensure a specific file does not exist, it is logical consistent to vacuously return success status if remove_path is invoked on a file that does not exist to start with, albeit if this behaviour is explicitly documented. The obviates the need for any additional method argument.

[balbirthomas]

Another way to view the alternative suggested above is to ask the question "when will it happen that a developer would not want to use the -f" option ? If the answer is never then a -f method argument is redundant.

[jameinel]

It depends. rm a missing file is usually an error because it is usually a sign of a typo and you should be made aware that what you thought would be removed isn't.

On Mon, Feb 21, 2022, 09:14 Balbir Thomas @.***> wrote:

By equating the behaviour of rm and remove_path @Abuelodelanada https://github.com/Abuelodelanada does make a convincing case why a -f would be useful, in the sense that it will unsurprising to the developer. However given the fact that the objective of remove_path is to ensure a specific file does not exist, it is logical consistent to vacuously return success status if remove_path is invoked on a file that does not exist to start with, albeit if this behaviour is explicitly documented.

— Reply to this email directly, view it on GitHub https://github.com/canonical/operator/issues/699#issuecomment-1046924291, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABRQ7J7RLUUJH4XQYV6XNTU4JCFRANCNFSM5OYVMC6A . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you are subscribed to this thread.Message ID: @.***>

[balbirthomas]

It depends. rm a missing file is usually an error because it is usually a sign of a typo

That is a good point and a valid concern indeed. However if a typo is made in a interactive command such as rm then a human being may notice it and fix it. But what is a charm supposed to do when it tries to delete a file and realizes it does not exist ? Go into blocked state ? If the charm goes into blocked state what is the system administrator supposed to do ? Alternatively a charm can just log the error and continue, in which case I see not any difference from the operator framework just logging the error instead of raising an exception or requiring an additional method argument.

[balbirthomas]

I suppose it is also a matter of what is going to be the common case.

1. A charm will want to delete a file and not care if it did not exist already.
2. A charm will want to delete a file and be informed if it did not exist already.

If 1 is the common case and the preference is to have an additional method argument akin to rm -f then perhaps the -f option should be the default.

[pengale]

I suspect that a lot of people using the lib would just pass -f, and create their own problems detecting typos.

I like the simplicity of having the library work one way, with my preferred way being to remove a path if it exists, and log a warning if it does not. (The warning gives typo makers a chance to catch their error.)

[benhoyt]

I think at this point we should simply "solve" this by documenting the existing behaviour -- making clear what exceptions are raised when, and how it's different if recursive=True.

Related: right now the docstrings have gotten out of sync between pebble.Client.remove_path and model.Container.remove_path, so we should get them back in sync too.

[amandahla]

I was about to file an issue regarding this. I missed the docs warning about how remove_path raises PathError.