-
Notifications
You must be signed in to change notification settings - Fork 127
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
feat: build environment variables #267
base: master
Are you sure you want to change the base?
Conversation
443ebac
to
0e77730
Compare
Hi @medikoo! Could you please review the PR when you have a time? |
Hello @shekhirin - sorry for not getting into your Pr sooner - could you please rebase it on top of current master and request a review from me? Thanks 🙇 |
0e77730
to
9d2ab1c
Compare
Hey @pgrzesik, just rebased! Seems like I don't have access to requesting a review 🤷♂️ |
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.
Thanks @shekhirin 🙇 It looks great in general, I only have one minor concern, please let me know what do you think
cloudFunctionBuildEnvironmentVariables: { | ||
type: 'object', | ||
patternProperties: { | ||
'^.*$': { type: 'string' }, |
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.
Should that regex really be this permissive? It accepts white spaces, etc, which I'm guessing might be not valid properties here.
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 haven't got permission to push to @shekhirin 's fork, but I believe a sane regex would be [a-zA-Z_][a-zA-Z0-9_]*
based on this answer and the first comment on it https://stackoverflow.com/a/2821183/9997939
Just found this while looking for a way to set Any chance of getting this merged anytime soon? Thanks! |
@shekhirin Maybe you need to rebase this to resolve conflicts? |
Google allows to update Build Environment Variables on deploy, so let's make use of such opportunity: https://cloud.google.com/functions/docs/env-var#updating_build_environment_variables.
buildEnvironmentVariables
property is from REST API documentation https://cloud.google.com/functions/docs/reference/rest/v1/projects.locations.functionsNow we have a better solution for private Go dependencies than having a
vendor
. With build env vars, you can setGOPROXY
and buildpack will fetch needed private dependencies through it. Yay.