-
Notifications
You must be signed in to change notification settings - Fork 671
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
helm: Add support for passing env variables to flyteadmin using envFrom #5216
helm: Add support for passing env variables to flyteadmin using envFrom #5216
Conversation
Thank you for opening this pull request! 🙌 These tips will help get your PR across the finish line:
|
Signed-off-by: Chris Soyars <[email protected]>
1cd6313
to
9ff19da
Compare
Signed-off-by: Chris Soyars <[email protected]>
fae3f59
to
acf00e7
Compare
Should we match
|
@ctso I think it would feel nicer if at least the value var names matched between single binary and flyte-core. yah? |
@ctso what are we missing to ship this? There are users dealing with limitations where this feature would be helpful |
I think this could be merged as-is, though it sounds like maybe we want to update the single binary chart to have the same behavior as well? |
@wild-endeavor not sure I follow, what you mean is matching in flyte-core the way this section enables envVars from different sources? flyte/charts/flyte-binary/templates/deployment.yaml Lines 169 to 177 in 0eaa20e
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5216 +/- ##
=======================================
Coverage 37.03% 37.03%
=======================================
Files 1318 1318
Lines 132267 132267
=======================================
+ Hits 48980 48986 +6
+ Misses 79028 79022 -6
Partials 4259 4259
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Congrats on merging your first pull request! 🎉 |
Tracking issue
Related to #3970
Why are the changes needed?
Users may want to pass environment variables from Secrets or ConfigMaps instead of relying on the Helm chart's
flyteadmin.secrets
Helm value. This is convenient when using a secrets manager such as External Secrets, for example.What changes were proposed in this pull request?
Add a new Helm value,
flyteadmin.envFrom
which you can pass through directly to the underlying Deployment.How was this patch tested?
Tested locally using
helm template
, it works.Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link