-
Notifications
You must be signed in to change notification settings - Fork 15
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 "Private" Environment Variables #103
Conversation
Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #103 +/- ##
==========================================
+ Coverage 55.53% 55.60% +0.07%
==========================================
Files 50 50
Lines 3065 3070 +5
==========================================
+ Hits 1702 1707 +5
Misses 1363 1363
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@property | ||
def env_private(self) -> Mapping[str, str]: | ||
return self._env_private | ||
|
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 really don't like that we have to keep changing this class and want to get to something that follows the open/closed principle instead, but this should do for the present purpose.
dbt_common/context.py
Outdated
self._env_secrets: Optional[List[str]] = None | ||
self._env_private = { | ||
k[len(PRIVATE_ENV_PREFIX) :]: v |
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.
what's the motivation for stripping the prefix off of private envs?
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.
My take is that it makes them less annoying and verbose to look up later, but I could go either way.
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 could go either way as well, generally anti-'magic' but this feels pretty minimal. Is there a good spot we can document the look up pattern?
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 think even the need to document it tilts me to the side of just keeping the prefix, so I'll just do it that way.
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.
Done!
Description
Support environment variables with a DBT_ENV_PRIVATE_ prefix, which dbt can see, but user code cannot.
Checklist
changie new
to create a changelog entry