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

Added Basic Shell Notifications #56

Merged
merged 1 commit into from
Dec 22, 2023

Conversation

CanYumusak
Copy link
Contributor

@CanYumusak CanYumusak commented Dec 21, 2023

Background

In order to make the fulfillment of requisitions easier, users are now able to configure a script that will be called when a new requisition has been created. This idea can expand to create a second requisition once the current requisition expires soon.

This idea was dicussed in #54 - this PR serves as a basis for discussion.

How to use

The config file supports a new property called NOTIFICATION_SCRIPT. This script is executed every time a new requisiton URL is created. The script receives the requisition link as a parameter.

Here's an example to post it to logsnag:

ynabber.env:

# Notifications
NOTIFICATION_SCRIPT=/Users/myuser/post-logsnag,sh

post-logsnag,sh:

#!/bin/bash

reqURL=$1
curl --location 'https://api.logsnag.com/v1/log' \
   --header 'Content-Type: application/json' \
   --header 'Authorization: Bearer {...Lognag API KEY...}' \
   --data-raw "{\"project\":\"myproject\",\"channel\":\"myChannel\",\"event\":\"Confirm requistion URL\",\"description\":\"${reqURL}\",\"icon\":\"\",\"notify\":true}"

@hpernu
Copy link
Collaborator

hpernu commented Dec 21, 2023

Not exactly against this but this solution is not best possible for docker usage.

You do not have the script file inside the image when run with docker. You can get visibility by using bind mounts but then the script must work inside the rather sparse image created by current build and it cannot access resources on the local file system. This may make debugging it harder and may create tickets claiming 'script does not work' when in fact the problem is that the user is effectively running the script inside docker. They may even use the path directly (without bind mount) and wonder why the script is not found!

I would propose a complimentary file which is only appended to. As I mentioned in the issue, this file can then be a named pipe(if the user wants so) and he can use a script (in a separately isolated environment) to read data from it.

But it's your code: I could work with either solution, should I need a notification mechanism. Although I can even now use output scraping ...

@CanYumusak
Copy link
Contributor Author

Not exactly against this but this solution is not best possible for docker usage.

You do not have the script file inside the image when run with docker. You can get visibility by using bind mounts but then the script must work inside the rather sparse image created by current build and it cannot access resources on the local file system. This may make debugging it harder and may create tickets claiming 'script does not work' when in fact the problem is that the user is effectively running the script inside docker. They may even use the path directly (without bind mount) and wonder why the script is not found!

I would propose a complimentary file which is only appended to. As I mentioned in the issue, this file can then be a named pipe(if the user wants so) and he can use a script (in a separately isolated environment) to read data from it.

But it's your code: I could work with either solution, should I need a notification mechanism. Although I can even now use output scraping ...

I honestly didn't even consider the docker issue you mentioned. Any idea on how to resolve it? Doesn't docker have access to the host environment?

Ref piped file: it requires some sort of file observation mechanisms in place that also need to run, when ynabber runs - which means that you now have to create a shell command that orchestrates launching both. All of that is feasible but seemed like a large overhead for the user. The reason might be lacking Unix knowledge from my side though. I'm happy to go that route if you could point me to some easy way to achieve this.

@hpernu
Copy link
Collaborator

hpernu commented Dec 22, 2023

Not exactly against this but this solution is not best possible for docker usage.
You do not have the script file inside the image when run with docker. You can get visibility by using bind mounts but then the script must work inside the rather sparse image created by current build and it cannot access resources on the local file system. This may make debugging it harder and may create tickets claiming 'script does not work' when in fact the problem is that the user is effectively running the script inside docker. They may even use the path directly (without bind mount) and wonder why the script is not found!
I would propose a complimentary file which is only appended to. As I mentioned in the issue, this file can then be a named pipe(if the user wants so) and he can use a script (in a separately isolated environment) to read data from it.
But it's your code: I could work with either solution, should I need a notification mechanism. Although I can even now use output scraping ...

I honestly didn't even consider the docker issue you mentioned. Any idea on how to resolve it? Doesn't docker have access to the host environment?

Docker container do not have access to the host system. That is kind of the idea of docker. They only get limited access by the usage of bind mounts, if the user so desires.

Using a script is fine. It is just likely unusable when running inside docker which is the safest approach to non-OS tools. ( And easiest too as there is no installation requirement to other tools or libraries - not to mention possible version issues )

There really isn't much that can be done to make it more usable with dockerized usage. There just isn't a way to do that. Consider the environment inside docker as an operating system but with only the bare minimum set of tools installed. Many containers do not even have a shell.

Ref piped file: it requires some sort of file observation mechanisms in place that also need to run, when ynabber runs - which means that you now have to create a shell command that orchestrates launching both. All of that is feasible but seemed like a large overhead for the user. The reason might be lacking Unix knowledge from my side though. I'm happy to go that route if you could point me to some easy way to achieve this.

Practically any UNIX tool (such as cat) able to read from stdin will easily work with named pipe without modifications. From the perspective of the program, it is just a file, which does not return EOF. Thus any program opening it for reading stays for data iuntil there is something. Tailing a file (i.e. a normal file which does have an actual ending) is a bit more complicated but can be circumvented by piping through tail -f .

A shell script can read from such a thing easily as well using the read-command.

But you're right that running a separate program requires a bit more in the way of wrapper scripts. I do not have a use for this functionality anyway as I am not running ynabber in daemon mode. I just let it query the bank once but run it every hour.

As I said, I advocate putting this option in anyway despite its limited usability when using docker. It is just that a complimentary approach (i.e. append to file) would be nice for people needing more. But then you can already find out this data from the logs. We just need to standardize the log format. Log following isn't that difficult.

If @martinohansen thinks this is find, let's merge it.

@martinohansen
Copy link
Owner

@CanYumusak thank you for taking the time to make this, I really appreciate it.

This will work just fine with Docker, it only requires that you mount the script into the container.

I want to make a few changes to this so I will merge your changes but won't cut a new release until I have made some adjustments, I hope that is okay. Feel free to use it as is by installing from main but do expect changes to how it works in the next release.

@CanYumusak
Copy link
Contributor Author

Thanks Martin. I would appreciate it if you could tag me in the adjustments PR!

@martinohansen martinohansen merged commit a38733f into martinohansen:main Dec 22, 2023
1 of 2 checks passed
@CanYumusak CanYumusak deleted the can/notifications branch December 25, 2023 10:13
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