feat: Add Posit product environment check helpers #391
Conversation
☂️ Python Coverage
Overall Coverage
New Files
Modified Files
|
| from __future__ import annotations | ||
|
|
||
| import os | ||
|
|
||
|
|
||
| def get_product() -> str | None: | ||
| """Returns the product name if called with a Posit product. | ||
|
|
||
| The products will always set the environment variable `POSIT_PRODUCT=<product-name>` | ||
| or `RSTUDIO_PRODUCT=<product-name>`. | ||
|
|
||
| RSTUDIO_PRODUCT is deprecated and acts as a fallback for backwards compatibility. | ||
| It is recommended to use POSIT_PRODUCT instead. | ||
| """ | ||
| return os.getenv("POSIT_PRODUCT") or os.getenv("RSTUDIO_PRODUCT") | ||
|
|
||
|
|
||
| def is_local() -> bool: | ||
| """Returns true if called while running locally.""" | ||
| return get_product() is None | ||
|
|
||
|
|
||
| def is_running_on_connect() -> bool: | ||
| """Returns true if called from a piece of content running on a Connect server.""" | ||
| return get_product() == "CONNECT" | ||
|
|
||
|
|
||
| def is_running_on_workbench() -> bool: | ||
| """Returns true if called from within a Workbench server.""" | ||
| return get_product() == "WORKBENCH" |
There was a problem hiding this comment.
Do we need to support dynamic environment variables here? Another option is to use static variables.
from __future__ import annotations
import os
POSIT_PRODUCT = os.getenv("POSIT_PRODUCT") or os.getenv("RSTUDIO_PRODUCT")
IS_POSIT_CONNECT = POSIT_PRODUCT == "CONNECT"
IS_POSIT_WORKBENCH = POSIT_PRODUCT == "WORKBENCH"Then from external.databricks/snowflake...
from .. import runtime
if not runtime.POSIT_PRODUCT:
return;Feel free to disregard this if you have already thought through it.
There was a problem hiding this comment.
this is great feedback. did not consider this.
i think i could go either way here. your approach is definitely simpler. let me ensure its testable. i would think just importing the module should trigger this to run but can check.
The connectapi and connectcreds equivalents are functions so followed suit but i like simple :)
There was a problem hiding this comment.
I think this might be a little misleading for users today since workbench doesn't support this var yet.
I haven't tested this on workbench yet but I'm expecting to do something like this:
posit-sdk-py/src/posit/connect/_utils.py
Lines 35 to 45 in a089705
There was a problem hiding this comment.
I think the functional approach is going to best here, especially from a testing perspective. Complicates things when the values are set upon import. Going to keep it functional.
@dbkegley yeah that is fine! can add these checks into the workbench runtime check
There was a problem hiding this comment.
@atheriel, do you have any thoughts here?
There was a problem hiding this comment.
Always the option punt and not have any workbench related check for now.
| return os.getenv("POSIT_PRODUCT") or os.getenv("RSTUDIO_PRODUCT") | ||
|
|
||
|
|
||
| def is_local() -> bool: |
There was a problem hiding this comment.
I suppose is_local is a little subjective since their application could run elsewhere that isn't their machine or Posit.
There was a problem hiding this comment.
i like your suggestion above. will leave deprecation warning in utils is_local but can map it use the global vars.
There was a problem hiding this comment.
going to keep this since staying with functional approach
|
@zackverham and @dbkegley after looking at the existing is_local function, it was only checking if POSIT_PRODUCT is not set to CONNECT. So it would be considered local if running on workbench. Is that the intended effect? |
That was the intent initially but now that we need a better way to differentiate between Connect vs Workbench vs elsewhere the original implementation no longer seems correct. |
Okay cool. This PR should address that then to ensure it is not connect nor workbench to be local. |
Fixes #389