dullage / flatnotes

A self-hosted, database-less note taking web app that utilises a flat folder of markdown files for storage.
MIT License
1.5k stars 87 forks source link

Add shell hooks #190

Open reddec opened 5 months ago

reddec commented 5 months ago

Adds shell hooks via environment variables FLATNOTES_HOOK_<name>.

In each option, %s will be replaced by path to note. Work dir is set to notes dir.

It opens opportunity to enable syncs. For example:

FLATNOTES_HOOK_CREATE="git add -A && git commit -m 'note %s' && git push"
FLATNOTES_HOOK_UPDATE="$FLATNOTES_HOOK_CREATE"
FLATNOTES_HOOK_DELETE="$FLATNOTES_HOOK_CREATE"
dullage commented 5 months ago

Thanks for the PR, I like the concept.

I'm tied up on other projects at the minute but will take a look as soon as I can.

reddec commented 5 months ago

@dullage thanks! Any updates?

reddec commented 5 months ago

A bit confused: @dullage == @clach04 ?

clach04 commented 5 months ago

A bit confused: @dullage == @clach04 ?

No, we are different people. I'm just a drive-by commenter who liked your PR ;-)

dullage commented 5 months ago

Sorry for the late reply. I have a few concerns with the current approach but hopefully a potential solution for them.

  1. The repo is laid out in a way which could (in the future) support different backends for note storage, attachment storage and authentication. For note storage, currently, the only option is file_system but this may change in the future and if it does, the shell hook code would need to be duplicated.
  2. Responses need to wait for the shell hook to complete, potentially slowing down the client.
  3. I haven't tested this one, but my suspicion is that if the shell hook errors, the API would return an error to the client even though the actual note task completed successfully.

To solve these, my suggestion is to look into the [Background Tasks)(https://fastapi.tiangolo.com/tutorial/background-tasks/) functionality of FastAPI. You may be able to use this to perform the shell hook after the response has been returned to the client.

Additionally, I think it would also be prudent to wrap the check_call in a try catch with the catch simply logging the error message using logger.error.

reddec commented 5 months ago

@dullage

  1. I believe the shell hooks are particularly beneficial when used in conjunction with file storage. For other remote storage solutions (e.g., S3) or databases, hooks can be easily implemented using native tools.
  2. This approach is intentional to maintain consistency across user operations. If a hook is executed in the background while another user operation occurs, it could lead to inconsistencies.
  3. Related to point 2, it's straightforward to bypass this with || true in the hook.

I completely understand and share your concerns. I apologize for not detailing these points in the PR description earlier. Given the specific use-case example, I couldn't identify a better alternative :man_shrugging: