-
-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
Flask server for interacting with the contrib bots through outgoing webhooks. #4864
Conversation
Things to keep in mind:
Edit: updated server address |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Edit): Oops, I left a comment in the wrong window!
create_users(zulip_realm, zulip_outgoing_bots, bot_type=UserProfile.OUTGOING_WEBHOOK_BOT) | ||
Service.objects.create(name="test", | ||
user_profile=get_user_profile_by_email("outgoing-webhook@zulip.com"), | ||
base_url="http://0.0.0.0:5002/bots/followup", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this 0.0.0.0:5002
? Seems like maybe we want it to be 127.0.0.1:5002
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay
c92248e
to
7643b6d
Compare
I merged a tweaked version of the outgoing webhook commit, so the basic outgoing webhook user should be available in the test suite now. I think we should do a bit of cleanup of the flask server system next. I think the main thing that this needs is a configuration system that we can use to move the API keys and other settings out of the flask server code and into a config file. I would suggest using a ConfigParser / zuliprc file style approach, similar to what our other bots do, though it might make sense to have a variation in the approach to support having multiple bots defined in the same file? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for merging the test commit, @timabbott ! @vabs22 , I think you can rebase so that this PR only has the Flask commit.
I left a few comments inline; otherwise, I agree with working on the configuration next. Please feel free to PM me if you want to discuss the format for configuration files, or if you need help with getting the ConfigParser
to work.
contrib_bots/contrib_server.py
Outdated
client=restricted_client, | ||
state_handler=state_handler) | ||
|
||
# return json_success("Success!") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be "Success!"
or json_success("Success!")
? Either way, can you please choose one and then remove the comment?
contrib_bots/contrib_server.py
Outdated
path = "bots/" + str(bot) + "/" + str(bot) + ".py" | ||
bots_lib_module[bot] = get_lib_module(path) | ||
|
||
class StateHandler(object): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think StateHandler
has been extracted and should be available now, without needing to redefine it here.
contrib_bots/contrib_server.py
Outdated
def test(bot): | ||
# type: (str) -> str | ||
# if bot not in available_bots: | ||
# return "requested bot service not supported" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, it's helpful for error messages to include more information about the error. In this case, for example, I would prefer something like this:
return "requested bot service {} not supported".format(bot)
contrib_bots/contrib_server.py
Outdated
return "" | ||
|
||
@app.route('/bots/<bot>', methods=['POST']) | ||
def test(bot): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Is test
the name you want here?
contrib_bots/contrib_server.py
Outdated
@app.route('/bots/<bot>', methods=['POST']) | ||
def test(bot): | ||
# type: (str) -> str | ||
# if bot not in available_bots: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please uncomment or remove?
contrib_bots/contrib_server.py
Outdated
state_handler = StateHandler() | ||
|
||
event = json.loads(request.data) | ||
message_handler.handle_message(message=event["message"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it would be helpful to add a comment about the expected format of the request somewhere (i.e. the fact that "message"
is the relevant key, as well as the expected format of event["message"]
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it would be helpful to add the comment. Currently we are sending the data directly but later on I think we would be passing it through an interface designed for this server. That might be the best place for it. Do you want me to add a TODO comment inside DoRestCall?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds good for now.
@timabbott @robot-dreams Thanks for you reviews!. I have incorporated the comments and updated it. For downloading the above file I have added a download button similar to existing one. It can be seen here: I have also updated the flask server to read the configuration file and then populate the |
5a720dd
to
39b5e16
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this is a nice approach to configuration! I left a few comments inline; please take a look.
static/js/settings_bots.js
Outdated
@@ -56,6 +56,14 @@ exports.generate_zuliprc_content = function (email, api_key) { | |||
"\n"; | |||
}; | |||
|
|||
exports.generate_zuliprc_content_for_single_file = function (email, api_key) { | |||
return "[" + email.substring(0, email.indexOf("-bot@zulip.localhost")) + "]" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this always be @zulip.localhost
? Would it make sense to use email.indexOf("-bot@")
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, would this code be more readable with a separate bot_name_from_email
function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with both of your points.
@@ -59,6 +59,13 @@ | |||
|
|||
<div class="form-horizontal" id="api_key_button_box"> | |||
<div class="input-group side-padded-container"> | |||
<div> | |||
<span>Download single zuliprc of all active bots</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could the zuliprc
name cause confusion because it's also used for other kinds of configuration files?
If so, would a name like zulipbotrc
or flaskbotrc
or (you can probably come up with something better) make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah flaskbotrc
would make more sense to the users.
contrib_bots/contrib_server.py
Outdated
app = Flask(__name__) | ||
|
||
@app.route('/') | ||
def main(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just checking, is it necessary to define this route / function?
If so, can it be called something besides main()
?
If not, can it be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had added this initially for testing, so it can be removed now. I should have removed it much earlier. My bad.
contrib_bots/contrib_server.py
Outdated
state_handler = StateHandler() | ||
|
||
event = json.loads(request.data) | ||
message_handler.handle_message(message=event["message"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds good for now.
contrib_bots/contrib_server.py
Outdated
|
||
from zulip import Client | ||
|
||
config_file_path = sys.argv[1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Referencing sys.argv[1]
at the top level like this seems like it could lead to confusion or problems—especially if this module gets imported somewhere else.
While this is pretty clearly a standalone app and probably won't ever be imported, I still think it's important to consistently use good habits across the codebase.
Instead of the current approach, do you think it would make sense to define a load_config
function that takes a file_path
as a parameter, and then call it from the if __name__ == "__main__"
block?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it would be much better to shift these code lines into separate functions and call them from if __name__ == "__main__"
block.
contrib_bots/contrib_server.py
Outdated
site=bots_config[bot]["site"]) | ||
restricted_client = BotHandlerApi(client) | ||
message_handler = bots_lib_module[bot].handler_class() | ||
state_handler = StateHandler() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add a TODO
to handle stateful bots (e.g. the Tic Tac Toe bot)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay
@@ -59,6 +59,13 @@ | |||
|
|||
<div class="form-horizontal" id="api_key_button_box"> | |||
<div class="input-group side-padded-container"> | |||
<div> | |||
<span>Download single zuliprc of all active bots</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be helpful to say what this is for (e.g. Download config of all active bots for Flask bot server
or something)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I agree.
d12e0ad
to
f663269
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thanks for addressing all of my comments! I can't wait to see this in action.
@vabs22 I merged the first commit to help make it easy for other folks to collaborate on the flask system, congrats on getting that done! I noticed a few things we'll want to fix in the near future:
@eeshangarg, as the maintainer of the More medium-term, we will want to work on these:
@vabs22 can you and @robot-dreams open issues for all of the above follow-up tasks that aren't going to be done this week? I imagine those will be your projects for the next week or two. For the JS changes, I opened #5355 for a related cleanup we'll want to do, which would be reasonable for you to work on @vabs22. The remaining patch in this PR I can merge once we fix a couple small things:
|
@vabs22 Can you clean up the commit message here? The title line should be no longer than 65 characters, and then you can have an empty line and more text to elaborate as needed. |
Flaskbotrc is a file containing config of all active outgoing webhook bots. It is used to provide configuration of all active outgoing webhook bots to zulip-bot-server.
@showell I have updated the commit message and the tests. Please take a look. |
Merged! Thanks, @vabs22! |
No description provided.