Set same env variables as sensor process and s/_sensor_service/sensor_service #2695
Conversation
lakshmi-kannan
force-pushed
the
env_variables_action
branch
from
0bd7fac to
8137e37
Compare
|
Docs: StackStorm/st2docs#157 |
lakshmi-kannan
changed the title
| :rtype: ``dict`` | ||
| """ | ||
| result = {} | ||
| result['ST2_ACTION_PACK_NAME'] = self.get_pack_name() |
There was a problem hiding this comment.
Let's please move this to a different place.
As I noted here #2693 (comment), there are two different things:
- Environment variables which are set for st2client and datastore service to work (as mentioned in the issue, that's a hack, I don't like it but that's limitation of st2client right now - that's an implementation detail of the way datastore service and st2client works)
- Environment variables which are set as a convenience and available to actions.
In this case we are fixing 1), but you are doing in in code for 2).
|
Let's please get this addressed - #2695 (comment) Besides that, LGTM, 👍 |
|
As mentioned in #2699 (yes, that PR I opened because I missed this one), it would also be good to add some (integration / end to end) tests which test datastore operations to st2tests or similar. The unit tests which we have right now only test implementation details - they test correct environment variables are set. What we really are interested in and should be testing is "do datastore related methods" work. For that, we need end to end tests. |
|
Minor comment wrt to PR title and description - imo, it's better if the PR title includes a more high-level description of what is being fixed / changed. This makes it easier for anyone checking the PR to understand what is going on and what is being changed. Right now it includes implementation detail ("setting same environment variables"and by itself this doesn't tell much). Imo, a better title would be something along the lines of "Fix datastore access in the Python runner actions" or something along those lines since that what we are doing - we are fixing datastore access for Python runner actions (and "setting same environment variables" is just an implementation detail of the fix which can be included in the description). |
|
As per slack discussion, here is an integration test for datastore access in action - StackStorm/st2tests#47 |
|
Thanks for moving the code and adding a test 👍 |
Fixes #2694 and #2693
Need feedback on StackStorm/st2docs#156