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

Add remote function support #986

Merged

Conversation

WilliamBergamin
Copy link
Contributor

@WilliamBergamin WilliamBergamin commented Nov 16, 2023

This PR aims to add minimalist support for remote functions in python

Feedback

I'm looking for feedback on

  • Usability, is this what we envisioned the developer experience would be?
  • Does this aline with the Bolt-js experience?
  • Implementation, does this implementation align with bolt principles
  • Tests, is this missing tests that could be valuable for the feature

Testing

I created this gist to show how to test this new feature, use it for the following steps

  1. Create a new app
    a. Head to https://api.slack.com/apps/?new_app=1
    b. Click "From an app manifest"
    c. Select YAML and paste the above manifest.yml content
    d. Click create button
  2. Install the app into org/workspace
    a. Install the app anyways (when the workspace is in an Org, org-wide installation is required to publish functions)
    b. Grab the xoxb- token (Settings > Install App > Bot User OAuth Token)
    c. export SLACK_BOT_TOKEN=xoxb-...
  3. Set up Socket Mode
    a. Head to Settings > Basic Information > App-Level Tokens on the https://api.slack.com/apps page
    b. Generate a new token with connections:write scope
    c. export SLACK_APP_TOKEN=xapp-...
  4. Spin up the app
    a. Add app.py and async_app.py with the above source code
    b. Pull the latest PR branch of bolt python to your local
    c. Run scripts/build_pypi_package.sh this will generate a .whl in the dist/ folder
    d. (Optional) In your project set up a venv with python -m venv .venv and source .venv/bin/activate
    e. pip install global/path/to/bolt-python/dist/something.whl
    f. pip install aiohttp
    g python app.py or python async_app.py
  5. Add the custom step to a workflow
    a. Open the workflow builder
    b. Create a new workflow with a link trigger
    d. Seach "Hello" in the Steps view on the right side
    e. Add the step to the workflow (you can set any user ID as the input)
    f. Publish the workflow

Category

  • slack_bolt.App and/or its core components
  • slack_bolt.async_app.AsyncApp and/or its core components
  • Adapters in slack_bolt.adapter
  • Document pages under /docs
  • Others

Requirements

Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you are agreeing to those rules.

  • I've read and understood the Contributing Guidelines and have done my best effort to follow them.
  • I've read and agree to the Code of Conduct.
  • I've run ./scripts/install_all_and_run_tests.sh after making the changes.

Copy link
Member

@seratch seratch 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 sharing this! This is already amazing but I left a few comments

slack_bolt/app/app.py Outdated Show resolved Hide resolved
slack_bolt/app/app.py Outdated Show resolved Hide resolved
slack_bolt/app/async_app.py Outdated Show resolved Hide resolved
slack_bolt/app/app.py Outdated Show resolved Hide resolved
slack_bolt/app/async_app.py Outdated Show resolved Hide resolved
slack_bolt/request/async_internals.py Show resolved Hide resolved
slack_bolt/request/internals.py Outdated Show resolved Hide resolved
slack_bolt/request/internals.py Show resolved Hide resolved
tests/scenario_tests/test_function.py Show resolved Hide resolved
tests/slack_bolt/request/test_internals.py Outdated Show resolved Hide resolved
Copy link
Contributor

@misscoded misscoded left a comment

Choose a reason for hiding this comment

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

Nice work! Looking good so far. 🎉

Usability, is this what we envisioned the developer experience would be?
Usability in the gist looks good, though I want to confirm that we're going to also support the same utilities currently available in the BoltJS implementation, correct? i.e., inputs, definition, complete, fail.

Does this aline with the Bolt-js experience?
So long as the same utilities are pulled over (see above) and we replace the low-level API calls (with formal methods (e.g., functions_completeSuccess), the experience looks generally aligned.

Implementation, does this implementation align with bolt principles
Enough, IMO, though Kaz to ultimately decide.

There are some differences to be expected between the two, but that’s OK. For example, the matchers aspect is nested within the WorkflowFunction class in Bolt, but what you have here appears to be the standard approach in Bolt for Python.

One thing: we have gone with a class-based design for tidiness/organization purposes (similar to WorkflowStep seen here). I ultimately leave it to Kaz to decide if he wants to carry this approach over.

Tests, is this missing tests that could be valuable for the feature
We currently have tests as a non-blocking requirement. That said, given how quickly we’re progressing thus far, I don’t see an issue with us ensuring tests are also introduced by EOQ.

Copy link
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

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

Hell yeah I love it!
The API looks great from my point of view - feels extremely familiar.
Worked like a charm too 🥇
This is really exciting!!!

@@ -103,13 +105,23 @@ def matches(self) -> Optional[Tuple]:
"""Returns all the matched parts in message listener's regexp"""
return self.get("matches")

@property
def function_execution_id(self) -> Optional[str]:
"""The `function_execution_id` associated with this request."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we document that this is only available for function_executed events and interactivity events scoped to a function?

Copy link
Contributor Author

@WilliamBergamin WilliamBergamin Nov 17, 2023

Choose a reason for hiding this comment

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

Good point 💯 I will add something like "only available for function related events"?

# --------------------------------

@property
def authorize_result(self) -> Optional[AuthorizeResult]:
"""The authorize result resolved for this request."""
return self.get("authorize_result")

@property
def function_bot_access_token(self) -> Optional[str]:
"""The bot token resolved for this function request."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above re: only available for function_executed events and interactivity events scoped to a function.

@@ -176,6 +177,17 @@ def _verify_message_event_type(event_type: str) -> None:
raise ValueError(error_message_event_type(event_type))


def function_event(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a docstring here to clarify what this is for? It is clear to me given the context, but I don't trust my memory to remember this in e.g. a year from now 😅

The event and construct here (Slack Custom Function) is also named in a way that makes it confusing when discussing it in a programming concept (are you talking about the Slack Custom Function, or a Python function? etc.). I think we need to be disciplined about terminology.

Copy link
Contributor

Choose a reason for hiding this comment

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

Technically the type of the specific event related here is called a function_executed event - perhaps tweaking the naming to match that makes sense too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like renaming this function to match the event type 💯 I agree that it aligns better with the terminology

@WilliamBergamin
Copy link
Contributor Author

FYI, PR #987 should fix the failing tests

@WilliamBergamin
Copy link
Contributor Author

@misscoded thanks for the feedback 🙇

I want to confirm that we're going to also support the same utilities currently available in the BoltJS implementation, correct? i.e., inputs, definition, complete, fail

We will definitely support inputs, complete and fail 💯 as for definition I need to confirm this is something we can extract from the payload, I think we can add these helpers in follow up PRs this should make it easier to review and priorities.
This PR aims to be the minimal implementation of the function handler

Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

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

looks great to me so far!

Copy link

codecov bot commented Nov 20, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (4b086cf) 91.76% compared to head (fba35ae) 91.96%.

Files Patch % Lines
slack_bolt/request/internals.py 93.10% 2 Missing ⚠️
slack_bolt/context/base_context.py 90.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #986      +/-   ##
==========================================
+ Coverage   91.76%   91.96%   +0.20%     
==========================================
  Files         181      190       +9     
  Lines        6312     6509     +197     
==========================================
+ Hits         5792     5986     +194     
- Misses        520      523       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@WilliamBergamin WilliamBergamin marked this pull request as ready for review November 20, 2023 20:01
@WilliamBergamin WilliamBergamin changed the title Introduce The Function Handler Add remote function support Nov 20, 2023
Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

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

while reviewing this PR again, i found a few minor issues

default_message,
f"""
@app.function("{callback_id}")
{'async ' if is_async else ''}def handle_{callback_id}_function(body, event, client, logger):
Copy link
Member

Choose a reason for hiding this comment

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

It seems that you can have "-" as part of callback_id. In this case, the generated code can have syntax error in Python. I haven't examined what else can be used for callback_id, so replacing all non-alphanumeric characters to "_" (underscore) should be a safe way to deal with such edge cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch 💯 I've opted to follow a similar pattern found in this file, by hardcoding the function name as handle_some_function instead of using the callback_id to dynamically name the function

slack_bolt/middleware/__init__.py Outdated Show resolved Hide resolved
@WilliamBergamin WilliamBergamin changed the base branch from main to feat-functions January 25, 2024 16:36
@WilliamBergamin
Copy link
Contributor Author

Changed the base branch from main to feat-functions in order to properly create a release candidate with the changes

@WilliamBergamin WilliamBergamin merged commit 370cddf into slackapi:feat-functions Jan 25, 2024
23 checks passed
@WilliamBergamin WilliamBergamin deleted the function_handler branch January 25, 2024 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants