Skip to content

Conversation

@sherif-fanous
Copy link
Contributor

What this Pull Request (PR) does

A Fabric pattern is made up of a directory in ~/.config/fabric/patterns where the name of the directory represents the pattern name. Within that directory are one or more .md files that describe the pattern.

With the current implementation that uses os.Remove, invoking the DELETE /patterns/:name API results in the following error

"could not delete test: remove /Users/sherif/.config/fabric/patterns/test: directory not empty"

since os.Remove will only delete files or empty directories.

This PR replaces os.Remove with os.RemoveAll that deletes the specified path and any children it contains

This ensures that patterns can be deleted whilst also maintaining functionality of being able to delete entities such as contexts/sessions

Copy link
Collaborator

@ksylvan ksylvan left a comment

Choose a reason for hiding this comment

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

I don't like the proposed behavior.

If Fabric finds something in the patterns directory that it didn't expect, it must have been created and placed there by the user, so it seems right for Fabric to fail with a helpful error message.

The user can then decide what to do with what they placed in the patterns/ directory and try again.

Copy link
Collaborator

@ksylvan ksylvan left a comment

Choose a reason for hiding this comment

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

I'm revising my thinking. If we assume that the API request is valid and authorized, then it should be okay to assume the user really wants to delete that pattern.

@sherif-fanous
Copy link
Contributor Author

sherif-fanous commented Apr 17, 2025

I understand the concern in case a user stores files unrelated to a pattern in a pattern directory.

With that said the current DELETE pattern API is technically of no use.

If the user will have to manually delete the files in the directory then why would they then use the API to delete the pattern directory?

I'd just do rm -fr ~/.config/fabric/patterns/test instead of going through the hassle of rm ~/.config/fabric/patterns/test/* then invoking the API

@ksylvan
Copy link
Collaborator

ksylvan commented Apr 17, 2025

With that said the current DELETE pattern API is technically of no use.

Agreed.

@eugeis eugeis merged commit 1645b0c into danielmiessler:main Apr 18, 2025
1 check passed
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.

3 participants