Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: implement basic hook logic #4
feat: implement basic hook logic #4
Changes from 14 commits
0da94c2
c6c1b31
a0782ab
a341b14
8023815
d69899e
3ea0ee0
0342720
02ef015
b8dd232
e94d26e
a9295e2
3fe2726
dd7114d
8c937ec
794e7f4
a3d272b
a3531f8
d076e86
904b489
50daa49
4965260
11c8984
309018e
c452feb
0744de6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
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.
Good point, this line is not required 💯
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 this require that the terminal session from where the CLI is executed already have the venv activated? If so, is there any way that we could have the hook activate the relevant venv automatically if it is not already active?
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.
This would be a great addition 💯 the issue is that the venv folder is arbitrarily named I follow the
env*.*.*
pattern but many name it.venv
,venv
,.env
....It might be easier to provide a proper error message is the hook fails?
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.
This could also be done in a follow up PR
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.
Ya sounds good, just thinking through how things could fail here.
My shell (oh-my-zsh) somehow understands when a venv is activated, as it prints the name of the venv in my shell prefix (screenshot attached).
It seems like the
activate
script sets an environment variable:Maybe the CLI hooks could look for the presence of this env var and if it is missing, error out but with a helpful message? Dunno, thinking out loud.
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.
The issue here is that using a
venv
is not enforced by python, from what I know using avenv
is considered good practice but a dev could chose to use no env/dependency manager or use an alternative like condaMaybe we could detect if the module is not installed on their path and ask to install it
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 don't have a lot of experience w/ building apps that use Python frameworks, but do application building frameworks in Python at all rely on venv? Like, are there frameworks that require developers to use venvs, or is that generally considered bad form in the Python community?
IMO it feels OK for us to require developers to use venv if they want to use Bolt, but that is just my opinion.
@seratch do you have opinions on this?
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 did a quick research on this and there is a push for virtualenv since Python comes bundled with the venv module to create virtual environments.
All recommend using venv but do not rely on it
VS code and PyCharm support many environment managers including venv and conda
It seems like if we rely only on venv we would be providing an opinionated experience (we could potentially support venv and conda)
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 that info and summary! Feel free to drop it - perhaps we can revisit in the future.
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 @WilliamBergamin on this > The issue here is that using a venv is not enforced by python, from what I know using a venv is considered good practice but a dev could chose to use no env/dependency manager or use an alternative like conda
Devs have freedom not to use venv for running apps (think about a Docker container with a specific Python version; just relying on the installed runtime is totally fine in the case). Great to see this discussion!
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.
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.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.
Good catch 💯 I tried implementing this but can't seem to get it to work, I've brought it up to the wider team
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.
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 PRsThere 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.
Totally fair! Thanks for following up on it.
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'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 theclean
method, these methods could be simple static functions 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.
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 programLet me know if this should be renamed or if there is a better implementation
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.
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:
You can adjust the details but I hope this psuedo code illustrates my idea well.
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.
Damn, this is clean! 💯
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.
👨🍳 💋