8go / matrix-eno-bot

An admin and personal assistence Matrix bot built on matrix-nio and nio-template
Apache License 2.0
88 stars 15 forks source link

Implemented Feature Request #18, provided sender to invoked script via environment variable #21

Closed troglodyne closed 3 years ago

troglodyne commented 3 years ago

hello.sh hijacked as a demonstration of the added capability.

@sender argument will be replaced with the requesting user, thus:

!c hello @sender

Would result in the same result as:

!c hello

Due to @sender already being appended as a "default" arg when nothing is supplied.

toushin-taishi commented 3 years ago

Hi @troglodyne,

Thanks for taking the time to look into this request. I tried your code and it works well when the script does not have an argument given. However, if the script includes an argument, it does not return the @sender as the $1 variable is replaced by the script's parameter. Is the result of my test correct?

troglodyne commented 3 years ago

Yes, the results of your test are correct.

If you wanted to still get the sender notified, they can pass in @sender as the argument to the bot instead.

I wanted to make this somewhat flexible regarding how someone may want to use it, so I thought allowing it as both a default arg and a "special" argument would be the way to go.

If, however, you are finding that passing in @sender to the bot does not produce the desired result, then by all means push back and I'll try to see what the bug is.

Otherwise, any other feedback would be appreciated as well. Though I did try to anticipate common use cases, I may not have gone about it in an optimal manner.

8go commented 3 years ago

@troglodyne thank you :star: so much for your efforts and for contributing this features to the community.

Please read the comments I left in issue #18.

troglodyne commented 3 years ago

OK! Testing revealed one thing I had to fix in the hello script example (which I have force pushed to correct) and another bug I was manually working around with unstaged changes on my server (none of the executable scripts were marked +x)

I've added the second fix as a separate commit in case you don't feel like you want/need that, though I saw it as an obvious fix so threw it in anyways.

Anyways, results are looking good (I have an "insult" script I used to test out a scenario where you want to "conditionally" tag the sender): gotem

8go commented 3 years ago

Excellent @troglodyne

Well done and thank you.

Please change the comments from: https://github.com/8go/matrix-eno-bot/pull/21/files#diff-3f39469e3559fd8199ed4e72e3a2e4c88cc51b8fb2595e99db26f8c18ed97783R182-R185

from

# Set any special ENV vars you want available to the process here.
# Note that subprocess.Popen *replaces* env here, so we have to augment
# the existing ENV for the daemon user to ensure more or less "expected"
# operation (important RE $PATH, etc).

to:

# Set environment variables for the subprocess here.
# Env variables like PATH, etc. are already set. In order to not lose
# any set env variables we must merge existing env variables with the
# new env variable(s). subprocess.Popen must be called with the
# complete combined list.

Once done I will merge your PR. :clap:

8go commented 3 years ago

Excellent @troglodyne

Well done and thank you.

Please change the comments from: https://github.com/8go/matrix-eno-bot/pull/21/files#diff-3f39469e3559fd8199ed4e72e3a2e4c88cc51b8fb2595e99db26f8c18ed97783R182-R185

from

# Set any special ENV vars you want available to the process here.
# Note that subprocess.Popen *replaces* env here, so we have to augment
# the existing ENV for the daemon user to ensure more or less "expected"
# operation (important RE $PATH, etc).

to:

# Set environment variables for the subprocess here.
# Env variables like PATH, etc. are already set. In order to not lose
# any set env variables we must merge existing env variables with the
# new env variable(s). subprocess.Popen must be called with the
# complete combined list.

Once done I will merge your PR. :clap:

troglodyne commented 3 years ago

Pushed updated comment, should be good to go now :+1:

8go commented 3 years ago

Lovely :heart: , perfect, thanks @troglodyne , :clap:

I already updated the README file to include this new feature!