feat: implement basic hook logic #4

WilliamBergamin · 2023-12-05T22:09:18Z

Summary

This PR aims to introduce an implementation of the Slack CLI interface that enables the Slack CLI to interact with Bolt Python projects

Testing

Follow the instructions in the README.md
When it asks to pip install slack-cli-hooks execute the following instead pip install -i https://test.pypi.org/simple/ slack-cli-hooks==0.0.0.dev1

Special notes

I debated implementing a more object oriented solution, opted for this since it felt simpler with less boiler plate, let me know what you think
Looking for feedback on code style and cleanliness
Did I miss anything?

Requirements

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_and_run_tests.sh after making the changes.

seratch

Great work! Left a few comments

seratch · 2023-12-05T23:39:53Z

.github/workflows/flake8.yml

@@ -11,7 +11,7 @@ jobs:
    timeout-minutes: 2
    strategy:
      matrix:
-        python-version: ["3.12"]
+        python-version: ["3.11"]


You can add a comment mentioning the reason behind this

I originally thought that flake8 did not support python 3.12 but flake8 6.1.0 has fixed this, I therefor update the format dependencies of the project

README.md

seratch · 2023-12-05T23:48:57Z

slack_cli_hooks/hooks/start.py

+SLACK_APP_TOKEN = "SLACK_APP_TOKEN"
+
+
+class EnvVarHandler:


I'd like to better understand the benefit of having this class. This class does destructive changes to real os.environ. In this case, is the state represented by self._is_set necessary? If the main benefit of this is to have the clean method, these methods could be simple static functions instead.

Good question, The goal of EnvVarHandler is to allow users to set their own environment variable values and not have them over-written and deleted by this hooks implementation

If a user sets the environment variable SLACK_APP_TOKEN then the app should use it and its value should still be present after the program terminates

self._is_set is necessary since it dictates wether the environment variable was set by the user prior to the execution of the program

Let me know if this should be renamed or if there is a better implementation

Ah, sorry I misread the code and now I understand the intention of it.

Allowing devs to go with their own env variables should be fine but we can make the underlying behavior more visible, especially when the CLI associated tokens are unused. When a dev maintains a few Slack apps, they can unintentionally have env variables like SLACK_BOT_TOKEN in a terminal shell session. How about adding logging outputs like your SLACK_BOT_TOKEN env variable is used over the CLI's tokens etc.?

Also, I'd suggest renaming the class and methods to be even clearer:

# Let's add "os" to avoid confusion with `slack env` os_env_vars = ManagedOSEnvVars() try: os_env_vars.set_if_absent(from=SLACK_CLI_XOXB, to=SLACK_BOT_TOKEN) os_env_vars.set_if_absent(from=SLACK_CLI_XAPP, to=SLACK_APP_TOKEN) finally: os_env_vars.clear()

You can adjust the details but I hope this psuedo code illustrates my idea well.

seratch · 2023-12-05T23:55:49Z

tests/mock_socket_mode_server.py

+from flask import Flask
+from flask_sockets import Sockets
+
+socket_mode_envelopes = json.dumps(


I haven't examined which patterns are actualy used but if many of the test data are unused, you can simplifty the code as much as possible.

Good point 💯 I've cleaned up this file to the minimal requirements

seratch · 2023-12-05T23:55:59Z

tests/mock_web_api_server.py

+        "error": "invalid_auth",
+    }
+
+    oauth_v2_access_response = json.dumps(


I haven't examined which patterns are actualy used but if many of the test data are unused, you can simplifty the code as much as possible.

Good point 💯 I've cleaned up this file to the minimal requirements

filmaj

Nicely done! Lots of progress here.

A majority of my comments are from a "README-driven development" perspective; as an outsider to the details of this project, I used the README to frame how I think about the project and that helps shape the project as well.

The test server is badass!

One question: I see we have "scenario tests" and "slack cli hooks" tests. I think the latter are unit tests. Is that right? And the former leverage the test server so are more integration style tests?

Another question re: testing: can we get code coverage reporting based on unit test execution?

README.md

filmaj · 2023-12-06T15:00:56Z

slack_cli_hooks/hooks/get_hooks.py

+        "start": f"{EXEC} -m slack_cli_hooks.hooks.start",
+    },
+    "config": {
+        "watcher": {"filter-regex": "^manifest\\.(json)$", "paths": ["."]},


Probably want to watch for .py changes too, right? Since Bolt will manage the websocket connection, it will operate as a single long-running process (as opposed to how the deno SDK operates, where every event starts a new deno process). Therefore, the Bolt process needs to be restarted every time app source code is changed to reflect the change.

Good catch 💯 I tried implementing this but can't seem to get it to work, I've brought it up to the wider team

Brining it up with the wider team reviled that the Slack CLI will not restart the start hook

I've decided to leave the "filter-regex": "^manifest\\.(json)$" since support for autoreload will be added in follow up PRs

Totally fair! Thanks for following up on it.

slack_cli_hooks/hooks/get_hooks.py

slack_cli_hooks/protocol/__init__.py

filmaj · 2023-12-06T15:17:26Z

slack_cli_hooks/protocol/default_protocol.py

+from .protocol import Protocol
+
+
+class DefaultProtocol(Protocol):


Damn, this is clean! 💯

filmaj · 2023-12-06T15:17:43Z

slack_cli_hooks/protocol/message_boundary_protocol.py

+        logging.error(msg, *args, **kwargs)
+
+    def respond(self, data: str):
+        print(self.boundary + data + self.boundary)


👨‍🍳 💋

Co-authored-by: Kazuhiro Sera <[email protected]>

Co-authored-by: Fil Maj <[email protected]>

tests/mock_socket_mode_server.py

tests/mock_web_api_server.py

WilliamBergamin · 2023-12-06T16:03:12Z

.github/workflows/flake8.yml

@@ -11,7 +11,7 @@ jobs:
    timeout-minutes: 2
    strategy:
      matrix:
-        python-version: ["3.12"]
+        python-version: ["3.11"]


I originally thought that flake8 did not support python 3.12 but flake8 6.1.0 has fixed this, I therefor update the format dependencies of the project

WilliamBergamin · 2023-12-06T16:14:02Z

slack_cli_hooks/hooks/start.py

+SLACK_APP_TOKEN = "SLACK_APP_TOKEN"
+
+
+class EnvVarHandler:


Good question, The goal of EnvVarHandler is to allow users to set their own environment variable values and not have them over-written and deleted by this hooks implementation

If a user sets the environment variable SLACK_APP_TOKEN then the app should use it and its value should still be present after the program terminates

self._is_set is necessary since it dictates wether the environment variable was set by the user prior to the execution of the program

Let me know if this should be renamed or if there is a better implementation

WilliamBergamin · 2023-12-06T19:15:40Z

tests/mock_socket_mode_server.py

+from flask import Flask
+from flask_sockets import Sockets
+
+socket_mode_envelopes = json.dumps(


Good point 💯 I've cleaned up this file to the minimal requirements

WilliamBergamin · 2023-12-06T19:15:42Z

tests/mock_web_api_server.py

+        "error": "invalid_auth",
+    }
+
+    oauth_v2_access_response = json.dumps(


Good point 💯 I've cleaned up this file to the minimal requirements

README.md

Co-authored-by: Fil Maj <[email protected]>

filmaj

Looks great! Thanks for addressing my comments.

filmaj · 2023-12-11T20:41:37Z

README.md


-app = App()
+## Simple project


Very nice! Eventually I imagine this entire section gets replaced with a sample, i.e. slack create -t slack-samples/bolt-starter-project or whatever.

filmaj · 2023-12-11T20:43:52Z

README.md

 Define the Slack CLI configuration in a file named `slack.json`.

 ```json
 {
  "hooks": {
-    "get-hooks": "python3 -m slack_cli_hooks.hooks.get_hooks"
+    "get-hooks": "python3 -X dev -m slack_cli_hooks.hooks.get_hooks"


Oooo things I learned today: the -X option

-X dev: enable Python Development Mode, introducing additional runtime checks that are too expensive to be enabled by default.
https://docs.python.org/3.12/using/cmdline.html#cmdoption-X

How much does the dev mode cost in terms of runtime performance? Since what the get-hooks does is fairly simple, it should be ignorable. That being said, we may should avoid recommending "dev" mode for production-deployed apps regardless of the cost. If we can enable this only for slack run (or it works only when running slack run), we don't need to worry about it at all though.

Actually, it's a good concern / callout @seratch , as the get-hooks hook gets invoked on every single CLI command.

Good call 💯 Adding it only to the start hook seems worth while, we can also remove it with minimal effect (but it might be harder to add later on)

seratch · 2023-12-12T03:59:46Z

README.md

 Define the Slack CLI configuration in a file named `slack.json`.

 ```json
 {
  "hooks": {
-    "get-hooks": "python3 -m slack_cli_hooks.hooks.get_hooks"
+    "get-hooks": "python3 -X dev -m slack_cli_hooks.hooks.get_hooks"


-X dev: enable Python Development Mode, introducing additional runtime checks that are too expensive to be enabled by default.
https://docs.python.org/3.12/using/cmdline.html#cmdoption-X

How much does the dev mode cost in terms of runtime performance? Since what the get-hooks does is fairly simple, it should be ignorable. That being said, we may should avoid recommending "dev" mode for production-deployed apps regardless of the cost. If we can enable this only for slack run (or it works only when running slack run), we don't need to worry about it at all though.

seratch · 2023-12-12T04:01:12Z

README.md

+  "settings": {
+    "org_deploy_enabled": true,
+    "socket_mode_enabled": true,
+    "token_rotation_enabled": false


Is this property required? If not, we can remove this line to make the example as simple as possible. I believe the default value for this is false.

Good point, this line is not required 💯

seratch · 2023-12-12T04:22:06Z

slack_cli_hooks/hooks/start.py

+SLACK_APP_TOKEN = "SLACK_APP_TOKEN"
+
+
+class EnvVarHandler:


Ah, sorry I misread the code and now I understand the intention of it.

Allowing devs to go with their own env variables should be fine but we can make the underlying behavior more visible, especially when the CLI associated tokens are unused. When a dev maintains a few Slack apps, they can unintentionally have env variables like SLACK_BOT_TOKEN in a terminal shell session. How about adding logging outputs like your SLACK_BOT_TOKEN env variable is used over the CLI's tokens etc.?

Also, I'd suggest renaming the class and methods to be even clearer:

# Let's add "os" to avoid confusion with `slack env` os_env_vars = ManagedOSEnvVars() try: os_env_vars.set_if_absent(from=SLACK_CLI_XOXB, to=SLACK_BOT_TOKEN) os_env_vars.set_if_absent(from=SLACK_CLI_XAPP, to=SLACK_APP_TOKEN) finally: os_env_vars.clear()

You can adjust the details but I hope this psuedo code illustrates my idea well.

slack_cli_hooks/protocol/__init__.py

seratch

Left two minor comments but this looks great to me now!

seratch · 2023-12-13T02:29:47Z

slack_cli_hooks/hooks/utils/managed_os_env_vars.py

+
+    def set_if_absent(self, os_env_var: str, value: str) -> None:
+        if os_env_var in os.environ:
+            self._protocol.warning(


Since this is not a problem, it'd be worth considering to go with info level instead. That being said, if only warn or above works in this scenario, warn level is fine too.

tests/slack_cli_hooks/hooks/utils/test_managed_os_env_vars.py

Co-authored-by: Kazuhiro Sera <[email protected]>

…k-hooks into hooks-logic

WilliamBergamin added 3 commits December 4, 2023 13:12

Add logic and tests

0da94c2

fix module

c6c1b31

update project

a0782ab

WilliamBergamin added the enhancement New feature or request label Dec 5, 2023

WilliamBergamin requested review from seratch and filmaj December 5, 2023 22:09

WilliamBergamin self-assigned this Dec 5, 2023

WilliamBergamin added 3 commits December 5, 2023 17:17

clean test utils

a341b14

Fix CI/CD for py 3.6

8023815

fix typo

d69899e

seratch requested changes Dec 5, 2023

View reviewed changes

filmaj suggested changes Dec 6, 2023

View reviewed changes

WilliamBergamin and others added 8 commits December 6, 2023 11:01

update flake8 version

3ea0ee0

Update README.md

0342720

Co-authored-by: Kazuhiro Sera <[email protected]>

Update README.md

02ef015

Co-authored-by: Fil Maj <[email protected]>

Simplify socketmode mock

b8dd232

Clean up mock web api server

e94d26e

Update README.md

a9295e2

Co-authored-by: Fil Maj <[email protected]>

Update README.md

3fe2726

Co-authored-by: Fil Maj <[email protected]>

Update README.md

dd7114d

Co-authored-by: Fil Maj <[email protected]>

WilliamBergamin commented Dec 6, 2023

View reviewed changes

WilliamBergamin and others added 5 commits December 6, 2023 16:23

Update README.md

8c937ec

Co-authored-by: Fil Maj <[email protected]>

Fix readme and watch

794e7f4

Remove .slack requirement

a3d272b

Adde watch regex test

a3531f8

Improve the readme based on feedback

d076e86

WilliamBergamin requested review from filmaj and seratch December 11, 2023 20:36

filmaj approved these changes Dec 11, 2023

View reviewed changes

seratch reviewed Dec 12, 2023

View reviewed changes

WilliamBergamin added 4 commits December 12, 2023 15:01

Fix dev mode

904b489

Simplify example

50daa49

Improve function naming

4965260

Improv env var handling

11c8984

seratch approved these changes Dec 13, 2023

View reviewed changes

WilliamBergamin and others added 3 commits December 13, 2023 10:08

Update tests/slack_cli_hooks/hooks/utils/test_managed_os_env_vars.py

309018e

Co-authored-by: Kazuhiro Sera <[email protected]>

Update managed_os_env_vars.py

c452feb

Merge branch 'hooks-logic' of https://github.com/slackapi/python-slac…

0744de6

…k-hooks into hooks-logic

WilliamBergamin merged commit 8e64ddb into main Dec 13, 2023
11 checks passed

WilliamBergamin deleted the hooks-logic branch December 13, 2023 15:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement basic hook logic #4

feat: implement basic hook logic #4

WilliamBergamin commented Dec 5, 2023

seratch left a comment

seratch Dec 5, 2023

WilliamBergamin Dec 6, 2023

seratch Dec 5, 2023

WilliamBergamin Dec 6, 2023

seratch Dec 12, 2023

seratch Dec 5, 2023

WilliamBergamin Dec 6, 2023

seratch Dec 5, 2023

WilliamBergamin Dec 6, 2023

filmaj left a comment

filmaj Dec 6, 2023

WilliamBergamin Dec 7, 2023

WilliamBergamin Dec 11, 2023

filmaj Dec 11, 2023

filmaj Dec 6, 2023

filmaj Dec 6, 2023

WilliamBergamin Dec 6, 2023

WilliamBergamin Dec 6, 2023

WilliamBergamin Dec 6, 2023

WilliamBergamin Dec 6, 2023

filmaj left a comment

filmaj Dec 11, 2023

filmaj Dec 11, 2023

seratch Dec 12, 2023

filmaj Dec 12, 2023

WilliamBergamin Dec 12, 2023

seratch Dec 12, 2023

seratch Dec 12, 2023

WilliamBergamin Dec 12, 2023

seratch Dec 12, 2023

seratch left a comment

seratch Dec 13, 2023

		from .protocol import Protocol


		class DefaultProtocol(Protocol):

feat: implement basic hook logic #4

feat: implement basic hook logic #4

Conversation

WilliamBergamin commented Dec 5, 2023

Summary

Testing

Special notes

Requirements

seratch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filmaj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filmaj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seratch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment