-
Notifications
You must be signed in to change notification settings - Fork 3
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
Add env vars for Workspace, Region and Branch #54
Conversation
// return default value if not found | ||
func getEnvVar(name string, defaultValue string) string { | ||
if val, found := os.LookupEnv(name); found { | ||
return val |
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 noticed that if the .env file doesn't exist we return an empty string instead of the default value, is that correct?
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.
Yes, that is correct. It's fine for now, as we only fetch an env var to bootstrap the connection params and if an empty string is encountered, continues to probe the .xatarc
file.
xata/utils.go
Outdated
// Get the region if the corresponding env var `XATA_REGION` is set | ||
// otherwise return the default region: us-east-1 | ||
func getRegion() string { | ||
return getEnvVar("XATA_REGION", defaultRegion) |
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.
nit: We appear to sometimes uses the raw string value of the environment variable names and sometimes constants. I think we should switch to constants everywhere. I'm happy to do this as a separate PR as doing it as part of this one could get messy.
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.
that's a good nit, I felt guilty not doing it :) I'll push it up in a sec and share the commit.
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.
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.
We still use the string values in a lot of tests. To get around this we'd have to make the constants public.
The workspace Id can be set via env var
XATA_WORKSPACE_ID
. This env var takes precedence over the.xatarc
file. TheXATA_WORKSPACE_ID
is present, there is an additional lookup for the env varsXATA_BRANCH
andXATA_REGION
.