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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add matrix-bot-chatgpt. #2386

Merged
merged 2 commits into from Jan 10, 2023
Merged

Add matrix-bot-chatgpt. #2386

merged 2 commits into from Jan 10, 2023

Conversation

bertybuttface
Copy link
Contributor

No description provided.

@bertybuttface
Copy link
Contributor Author

Copy link
Owner

@spantaleev spantaleev left a comment

Choose a reason for hiding this comment

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

Thanks for working on adding this bot!

I've submited some feedback. Also, it'd be nice to include some documentation (its own docs/ page that says how to best set it up with regards to authentication, etc). It'd be nice to update README.md and docs/configuring-playbook.md as well with mentions of this new bot.

roles/custom/matrix-bot-chatgpt/defaults/main.yml Outdated Show resolved Hide resolved
roles/custom/matrix-bot-chatgpt/defaults/main.yml Outdated Show resolved Hide resolved
@bertybuttface
Copy link
Contributor Author

Thanks for working on adding this bot!

I've submited some feedback. Also, it'd be nice to include some documentation (its own docs/ page that says how to best set it up with regards to authentication, etc). It'd be nice to update README.md and docs/configuring-playbook.md as well with mentions of this new bot.

Thanks for the feedback, I'll get working on it. I'll also add some basic docs

@bertybuttface
Copy link
Contributor Author

We also need to add the needed user to ensure-matrix-users-created playbook

@spantaleev
Copy link
Owner

We also need to add the needed user to ensure-matrix-users-created playbook

Oh, yes.. That'd be great! Some bot.chatgpt username would be consistent with all the other bot names. We'd need to use the "password" field (matrix_bot_chatgpt_matrix_bot_password - auto-populated based on the generic secret), not the access token.

@bertybuttface
Copy link
Contributor Author

Oh, yes.. That'd be great! Some bot.chatgpt username would be consistent with all the other bot names.

That's easy enough, I'll make the change.

We'd need to use the "password" field (matrix_bot_chatgpt_matrix_bot_password - auto-populated based on the generic secret), not the access token.

Ah, right now that won't work, logging in with a username and a password prints an access token, I can change that though if thats not very good expected behaviour.

@spantaleev
Copy link
Owner

The playbook can register a user with some password and then the bot could start with those credentials automatically.

If an access token is needed, the user would need to manually log in at least once to obtain the token.. and then edit vars.yml, and then rebuild and restart. It's much more involved.

@bertybuttface
Copy link
Contributor Author

The playbook can register a user with some password and then the bot could start with those credentials automatically.

If an access token is needed, the user would need to manually log in at least once to obtain the token.. and then edit vars.yml, and then rebuild and restart. It's much more involved.

I'll need to modify the upstream package (I have push permission) to cache the access token I think otherwise we would generate a new login each time. That seems to confuse the encrypted message support in matrix-bot-sdk.

@spantaleev
Copy link
Owner

If you could do that, that'd be great and seamless!

Alternatively, we can ask users to register the bot manually, log in manually to obtain the access token, then populate the config.. and finally do install-... + start. It's more of a hassle, but we also have lots of roles that do it this way.

Another alternative may be to make the bot an appservice and it register itself.

@bertybuttface
Copy link
Contributor Author

Alternatively, we can ask users to register the bot manually, log in manually to obtain the access token, then populate the config.. and finally do install-... + start. It's more of a hassle, but we also have lots of roles that do it this way.

Ok let's do this to get it merged and then I'll take a proper look at doing some cacheing.

@spantaleev
Copy link
Owner

We're in no rush to merge this. Maybe it's better if we do it right from the beginning, so we won't have to redo it twice and potentially think of backward compatibility when we change things.

Looks like the documentation page hasn't been updated yet? It assumes that ensure-matrix-users-created would do anything, but it can't because group_vars/matrix_servers hasn't been updated. It also mentions buscarron, etc.

I think it'd be best if you could make it work with a password (and cached access token) upstream, and then for us to jump straight to that.

@bertybuttface
Copy link
Contributor Author

Not rushing, ratcheting. I'd rather have a completed solution now and a better solution later potentially than have 80% of a better solution but end up with nothing.

I've made the changes but I have no idea when my schedule will allow me to work on cacheing. I don't think we leave this open indefinitely until then.

Co-Authored-By: Slavi Pantaleev <slavi@devture.com>
@bertybuttface
Copy link
Contributor Author

Fixed the ansible-lint errors, quite a few instances of yaml[comments]: Too few spaces before comment

Do I need to do anything about the warnings e.g.

key-order[task]: You can improve the task key order to: tags, block (warning)
roles/custom/matrix-bot-chatgpt/tasks/main.yml:3 Task/Handler: block/always/rescue 

key-order[task]: You can improve the task key order to: tags, block (warning)
roles/custom/matrix-bot-chatgpt/tasks/main.yml:15 Task/Handler: block/always/rescue 

args[module]: Unsupported parameters for ansible.builtin.service module: daemon_reload. Supported parameters include: state, arguments (args), pattern, name, sleep, enabled, runlevel. (warning)
roles/custom/matrix-bot-chatgpt/tasks/setup_uninstall.yml:10 Task/Handler: Ensure matrix-chatgpt is stopped

I based this on the matrix-bot-postmoogle code so if this is wrong then that is also wrong.

@spantaleev spantaleev merged commit 0b88293 into spantaleev:master Jan 10, 2023
spantaleev added a commit that referenced this pull request Jan 10, 2023
@spantaleev
Copy link
Owner

You can ignore the warnings. I've fixed up some additional things and merged this! Thank you!

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.

None yet

2 participants