-
Notifications
You must be signed in to change notification settings - Fork 0
[ENV VARS] Read env vars from user input #86
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
Conversation
Does the Pydantic model not pick up the env vars? It claims it does it in the documentation. See https://docs.pydantic.dev/latest/concepts/pydantic_settings/#dotenv-env-support |
It throws an error:
and I get this error:
but If I do this, it works fine
Also, another reason is the variable CENTML_CONFIG_PATH, this adds flexibility from where you call the script, since when you use github actions I don't know for sure whats the path and I don't think $(pwd) is allowed in .env file |
My comment meant that the current configuration should be able to pick up the env vars if you set them like
If it doesn't work, I think it's a bug in how we set up the configuration |
So I tested with new environment and test with main branch, and does not seem to pick up the env vars.
Credential File
|
That's annoying I guess pydantic doesn't support it well. |
centml/sdk/config.py
Outdated
CENTML_CONFIG_PATH: str = os.path.expanduser("~/.centml") | ||
CENTML_CRED_FILE: str = "credentials.json" | ||
CENTML_CRED_FILE_PATH: str = CENTML_CONFIG_PATH + "/" + CENTML_CRED_FILE | ||
CENTML_WEB_URL: str = os.getenv("CENTML_WEB_URL") or "https://app.centml.com/" |
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 would change this to be the following format os.getenv(ENV_VAR, default=DEFAULT_VALUE) instead of using or
s
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.
Yeap, done
Merging, I don't know if unit test are working right now |
Yeah it's a known issue all good to merge |
This helps with reading env vars from user input through:
export VAR=VAL or
VAR=VAL python script.py