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

fix: improve which python executable is being used #45

Merged
merged 4 commits into from
Jun 5, 2024

Conversation

WilliamBergamin
Copy link
Contributor

@WilliamBergamin WilliamBergamin commented Jun 3, 2024

Summary

This PR changes which python executable is used to populate the get-hooks payload.
This PR sets the python executable of other hooks command to the python executable used to run the get-hooks hook, defined in the slack.json file.

This allows developers to select which python executable the CLI must use to invoke hooks. This is accomplished by changing which executable is used to invoke the get-hooks hook. Here are some examples of "get-hooks" values.

Examples:

"get-hooks": "python3 -m slack_cli_hooks.hooks.get_hooks"
"get-hooks": "python2 -m slack_cli_hooks.hooks.get_hooks" // this is an example do not use python2
"get-hooks": "pypy -m slack_cli_hooks.hooks.get_hooks"
"get-hooks": "path/to/a/specific/bin/python -m slack_cli_hooks.hooks.get_hooks"

Testing

  1. pull this branch
  2. run scripts/build_pypi_package.sh
  3. use the path to the generated .whl file to import the changes in a python project
  4. In a python project run pip install /path/to/python-slack-hooks/dist/slack_cli_hooks-0.0.1-py3-none-any.whl
  5. run python3 -m slack_cli_hooks.hooks.get_hooks
  6. The the hooks values from the output should resemble "/Users/bob/path/to/my/project/env_3.12.2/bin/python -m slack_cli_hooks.hooks.get_manifest" and not "python3 -m slack_cli_hooks.hooks.get_manifest"

Special notes

Here are the scenarios I've tested

  • unit tests 🟢
  • Running the app with a global python executable using the python3 alias in slack.json 🟢
  • Running the app with a global python executable using the python alias in slack.json 🟢
  • Running the app with a virtual environment set up and the python3 alias in slack.json 🟢
  • Running the app with a virtual environment set up and the python alias in slack.json 🟢
  • Running the app with no virtual environment and the /Users/bob/path/to/virtural/env/bolt-python-project/env_3.12.2/bin/python alias set in slack.json 🟢
  • Running the app with a virtual environment set up and using the python executable of a different virtual environment in the slack.json 🟢
  • Running the app with a virtual environment set up but setting the python executable of a different virtual environment for the "start" hook only 🟢
  • Running the app with a pypy virtual environment using the python3 alias in slack.json 🟢
  • Running the app with no virtual environment and using the python executable of a pypy virtual environment in the slack.json 🟢
  • Running the app with a virtual environment set up that contains a space in the python executable path, ex: ./env 3.10.7/bin/python using the python3 alias in slack.json 🟢
  • Using PowerShell to run the app with a virtual environment set up that contains a space in the python executable path, ex: ./env 3.10.7/bin/python using the python3 alias in slack.json 🟢

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.

@WilliamBergamin WilliamBergamin added the bug Something isn't working label Jun 3, 2024
@WilliamBergamin WilliamBergamin self-assigned this Jun 3, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jun 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.58%. Comparing base (ea5748a) to head (ae8a3f1).

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #45      +/-   ##
==========================================
+ Coverage   94.55%   94.58%   +0.02%     
==========================================
  Files          11       11              
  Lines         202      203       +1     
==========================================
+ Hits          191      192       +1     
  Misses         11       11              

☔ 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 June 4, 2024 13:51
Copy link
Member

@zimeg zimeg left a comment

Choose a reason for hiding this comment

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

This might be one of my favorite PRs - such an elegant solution to something that's gone through so much discussion. And it works so dang cleanly 😭

Tested this in a virtualenv with both python3 and python and even pyhon and these all work as I'd hope. pyhon fails and the other two show the correct executable path in debug logs:

[2024-06-04 10:54:29] initialized SDK CLI config: {Runtime:python Hooks:{BuildProject:{Command: Name:BuildProject} CheckUpdate:{Command:/Users/ethan.zimbelman/programming/sandbox/trusting-armadillo-332/.venv/bin/python -m slack_cli_hooks.hooks.check_update Name:CheckUpdate} Deploy:{Command: Name:Deploy} Doctor:{Comma
nd:/Users/ethan.zimbelman/programming/sandbox/trusting-armadillo-332/.venv/bin/python -m slack_cli_hooks.hooks.doctor Name:Doctor} GetHooks:{Command:python -m slack_cli_hooks.hooks.get_hooks Name:GetHooks} GetManifest:{Command:/Users/ethan.zimbelman/programming/sandbox/trusting-armadillo-332/.venv/bin/python -m slack
_cli_hooks.hooks.get_manifest Name:GetManifest} GetTrigger:{Command: Name:GetTrigger} InstallUpdate:{Command: Name:InstallUpdate} Start:{Command:/Users/et
han.zimbelman/programming/sandbox/trusting-armadillo-332/.venv/bin/python -X dev -m slack_cli_hooks.hooks.start Name:Start}} Config:{Watch:{FilterRegex:(^manifest\.json$) Paths:[.]} SDKManagedConnection:true TriggerPaths:[] SupportedProtocols:[message-boundaries default]} WorkingDirectory:}

I let this simmer in thought, and this solution seems to cover things so well. A single update to slack.json seems reasonable to ask folks to make if needed IMO!

The only concern I have is around sharing this customized slack.json with teammates, but it's my understanding that a similar build tool or python executable is used by the entire team, so this change could be shared easily.

I'm like, actually so excited to see this 🏆 🚀 LGTM but am curious about hidden edges with this approach, if any exist 👀

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.

Thank you a thousand times for testing those scenarios so thoroughly ! That was exactly the kind of stuff I was going to test myself to see how it functions. The fact this is compatible with both global and virtualenv binaries is 💯

And mirroring @zimeg's comments: super elegant solution. As for the collaboration angle, I feel like this is where a team working on an app would use environment variables as part of their own setup: set some env var pointing to the relevant python binary, and then encode the name for that env var in the version-controled app slack.json. Different teammates don't need to tweak the slack.json and that can remain static, but each teammate can override the env var to suit their particular machine's needs. Done!

@WilliamBergamin
Copy link
Contributor Author

🙇 thank you
I agree with @filmaj, this implementation lets teams working on an app use environment variables or other strategies to customize the python executable for their given systems

But as @zimeg pointed out I am also curious about hidden edges with this approach, I will wait for @seratch review that may or may not uncover some edges

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.

This is great! Left one suggestion for an edge case 😉

slack_cli_hooks/hooks/get_hooks.py Outdated Show resolved Hide resolved
@WilliamBergamin WilliamBergamin merged commit 74e2d92 into main Jun 5, 2024
11 checks passed
@WilliamBergamin WilliamBergamin deleted the imporve_executable branch June 5, 2024 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants