Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add shell hooks #190

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Add shell hooks #190

wants to merge 1 commit into from

Conversation

reddec
Copy link

@reddec reddec commented May 28, 2024

Adds shell hooks via environment variables FLATNOTES_HOOK_<name>.

  • FLATNOTES_HOOK_CREATE - executes once note created
  • FLATNOTES_HOOK_UPDATE - executes once note updated
  • FLATNOTES_HOOK_DELETE - executes once note removed

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
Copy link
Owner

dullage commented May 30, 2024

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
Copy link
Author

reddec commented Jun 1, 2024

@dullage thanks! Any updates?

Copy link

@clach04 clach04 left a comment

Choose a reason for hiding this comment

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

lgtm - I've not tested this out but when I first saw this project this first thing I wondered was would this fit into my existing git backed notes directory.

A later change may need to deal with error reporting to the UI, so the user knows something happened. For example with git, if a conflict has taken place and need manual merging.


Requirements:

- python3, node, npm, pipenv
Copy link

Choose a reason for hiding this comment

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

👍 thanks for doc'ing this. I was thinking about experimenting with this so this will help!

@@ -3,6 +3,7 @@
import re
import shutil
import time
import subprocess
Copy link

Choose a reason for hiding this comment

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

Pedantic suggestion; recommend moving this to the line above to have pep-8 sorting. I know the existing code isn't strictly adhering to this. https://pypi.org/project/isort/ can help with this.

@@ -42,6 +42,7 @@ Equally, the only thing flatnotes caches is the search index and that's incremen
* Light/dark themes.
* Multiple authentication options (none, read-only, username/password, 2FA).
* Restful API.
* Hook on create/update/delete.
Copy link

Choose a reason for hiding this comment

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

Recommend a new section to provide a demo like you did in the PR.

Alternative suggestion for git, sample. 100% Untested :-) I used this technique in some of my (shell) scripts I removed the git add -A to also handle deletes and possible renames (rename detection can be a little hit and miss)

FLATNOTES_HOOK_CREATE="git add -u :/ ; git add . && git commit -m 'note %s' && git push"
FLATNOTES_HOOK_UPDATE="$FLATNOTES_HOOK_CREATE"
FLATNOTES_HOOK_DELETE="$FLATNOTES_HOOK_CREATE"


1. Install deps:

pipenv install
Copy link

Choose a reason for hiding this comment

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

Possibly dumb question; Should this include --ignore-pipfile?

RUN pipenv install --deploy --ignore-pipfile --system && \
uses it.

From https://pipenv.pypa.io/en/latest/commands.html#install

--ignore-pipfile — Install from the Pipfile.lock and completely ignore Pipfile information.

@reddec
Copy link
Author

reddec commented Jun 10, 2024

A bit confused: @dullage == @clach04 ?

@clach04
Copy link

clach04 commented Jun 10, 2024

A bit confused: @dullage == @clach04 ?

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

@dullage
Copy link
Owner

dullage commented Jun 12, 2024

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
Copy link
Author

reddec commented Jun 21, 2024

@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 🤷‍♂️

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