Skip to content

Make Telegram optional#501

Merged
EniasCailliau merged 3 commits into
mainfrom
ec/optional-telegram
Jul 27, 2023
Merged

Make Telegram optional#501
EniasCailliau merged 3 commits into
mainfrom
ec/optional-telegram

Conversation

@EniasCailliau
Copy link
Copy Markdown
Contributor

This PR makes the bot_token optional when using the TelegramMixin.

  • bot_token can be updated with the auth-protected connect_telegram route
  • bot_tokens are either passed via the internal kv store OR the config
  • config bot_token takes precedence over the kv-stored bot_token
  • Note: kv store is used since config is idempotent.

@EniasCailliau EniasCailliau requested a review from dkolas July 26, 2023 14:52
Enias Cailliau added 2 commits July 26, 2023 17:03
Copy link
Copy Markdown
Contributor

@dkolas dkolas left a comment

Choose a reason for hiding this comment

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

I definitely see value in being able to update the bot token and/or reconnect the instance to Telegram. What's the use case you're thinking of for not having to provide one at init time?

Also- I think maybe this change won't work the way it's currently written. The __init__ method allows using bot_token from the KV store if config.bot_token is None, but config.bot_token CAN'T be none, as all config fields must be filled out.

@EniasCailliau
Copy link
Copy Markdown
Contributor Author

I definitely see value in being able to update the bot token and/or reconnect the instance to Telegram. What's the use case you're thinking of for not having to provide one at init time?

Also- I think maybe this change won't work the way it's currently written. The __init__ method allows using bot_token from the KV store if config.bot_token is None, but config.bot_token CAN'T be none, as all config fields must be filled out.

@dkolas Oh. You we're on holidays I think when I showed a demo. Should have included that in the description.

Use case is:

  • You deploy an agent in an incomplete state. In my demo the agent didn't ingest youtube video's yet.
  • You use the agent's interface to complete the state. In the demo I added 10 video's of youtube
  • Then you test your agent. And think Ok I like it.
  • Only then do you connect/update your telegram bot.

@EniasCailliau
Copy link
Copy Markdown
Contributor Author

CAN'T

@dkolas The or-statement also catches the empty string which is our default "empty" value.

@EniasCailliau EniasCailliau added this pull request to the merge queue Jul 27, 2023
from steamship.utils.kv_store import KeyValueStore


class TelegramTransportConfig(Config):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would we want to provide a default '' string here for safety, simulate that it's now optional?

@eob
Copy link
Copy Markdown
Contributor

eob commented Jul 27, 2023

With apologies for slowing down the PR -- might be good to include a test in here since the Telegram code already has great test coverage.

It'd probably be akin to just repeating the existing tests, but:

  1. Not providing the telegram key upon init, never setting it, and then verifying that nothing works.
  2. Providing the telegram key upon init, setting it, then verifying everything works

@dkolas the convo was both that from a development perspective (it's easier to just test things w/o telegram involved) and a user perspective (why not add every transport and then let my [bot creator's] users pick which transport they want) it's nice to support late binding

Merged via the queue into main with commit a7fd0e4 Jul 27, 2023
@douglas-reid douglas-reid deleted the ec/optional-telegram branch July 28, 2023 14:15
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