-
Notifications
You must be signed in to change notification settings - Fork 670
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 k8s env froms #4969
Add k8s env froms #4969
Conversation
Signed-off-by: Neil Stout <[email protected]>
Signed-off-by: Neil Stout <[email protected]>
Signed-off-by: Neil Stout <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4969 +/- ##
==========================================
+ Coverage 58.97% 58.99% +0.01%
==========================================
Files 645 645
Lines 55578 55590 +12
==========================================
+ Hits 32778 32794 +16
+ Misses 20207 20202 -5
- Partials 2593 2594 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
85c2c35
to
7418d49
Compare
Signed-off-by: Neil Stout <[email protected]>
Signed-off-by: Neil Stout <[email protected]>
Signed-off-by: Neil Stout <[email protected]>
Signed-off-by: Neil Stout <[email protected]>
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 need a little bit more work to load configmaps and secrets.
flyteplugins/go/tasks/pluginmachinery/flytek8s/k8s_resource_adds.go
Outdated
Show resolved
Hide resolved
Signed-off-by: Neil Stout <[email protected]>
Signed-off-by: Neil Stout <[email protected]>
Signed-off-by: Neil Stout <[email protected]>
Signed-off-by: Neil Stout <[email protected]>
Signed-off-by: Neil Stout <[email protected]>
13c4188
to
0841046
Compare
Signed-off-by: Neil Stout <[email protected]>
@neilisaur , can you fix the lint error and merge master while you're at it? |
Signed-off-by: Neil Stout <[email protected]>
Signed-off-by: Neil Stout <[email protected]>
@eapolinario done and done! Re-ran make helm as well |
@eapolinario for the Chart version update that's failing is there a specific method of changing it, or should I just update everything under charts/ to v0.1.11? |
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.
@neilisaur , don't worry about that CI check failure. We've started to validate changes to the helm charts only recently, which means that we shouldn't be blocking PRs using those checks yet.
Thanks you for your contribution! |
* Adding allowance for default configMap/secrets to pull env froms Signed-off-by: Neil Stout <[email protected]> * Updating Helm charts Signed-off-by: Neil Stout <[email protected]> * Fixing typos Signed-off-by: Neil Stout <[email protected]> * More typo Signed-off-by: Neil Stout <[email protected]> * Ran 'make helm' Signed-off-by: Neil Stout <[email protected]> * Updating docs from `make helm` Signed-off-by: Neil Stout <[email protected]> * `make helm` results Signed-off-by: Neil Stout <[email protected]> * Fixing issues with env vs envFrom Signed-off-by: Neil Stout <[email protected]> * Fixing merge issue Signed-off-by: Neil Stout <[email protected]> * Better naming for variable Signed-off-by: Neil Stout <[email protected]> * Renaming to be more accurate Signed-off-by: Neil Stout <[email protected]> * Fixing typo Signed-off-by: Neil Stout <[email protected]> * Forgot one change Signed-off-by: Neil Stout <[email protected]> * Fixing linting error Signed-off-by: Neil Stout <[email protected]> * Updating Helm charts Signed-off-by: Neil Stout <[email protected]> --------- Signed-off-by: Neil Stout <[email protected]>
@neilisaur @eapolinario seems this broke tasks which already have I have a pod template something like: primary_container = V1Container(name="primary")
primary_container.env_from = [V1EnvFromSource(secret_ref=V1SecretEnvSource(name="my-secret"))]
ps = V1PodSpec(containers=[primary_container])
pod_template = PodTemplate(pod_spec=ps, primary_container_name="primary") |
* keep EnvFrom from pod template not complete nor tested, just as hint for potential fix for regression in #4969 * Add test and fix build error Signed-off-by: Eduardo Apolinario <[email protected]> * Fix test Signed-off-by: Eduardo Apolinario <[email protected]> --------- Signed-off-by: Eduardo Apolinario <[email protected]> Co-authored-by: Eduardo Apolinario <[email protected]>
* keep EnvFrom from pod template not complete nor tested, just as hint for potential fix for regression in flyteorg#4969 * Add test and fix build error Signed-off-by: Eduardo Apolinario <[email protected]> * Fix test Signed-off-by: Eduardo Apolinario <[email protected]> --------- Signed-off-by: Eduardo Apolinario <[email protected]> Co-authored-by: Eduardo Apolinario <[email protected]>
* keep EnvFrom from pod template not complete nor tested, just as hint for potential fix for regression in flyteorg#4969 * Add test and fix build error Signed-off-by: Eduardo Apolinario <[email protected]> * Fix test Signed-off-by: Eduardo Apolinario <[email protected]> --------- Signed-off-by: Eduardo Apolinario <[email protected]> Co-authored-by: Eduardo Apolinario <[email protected]> Signed-off-by: Vladyslav Libov <[email protected]>
Tracking issue
Closes #4968
Why are the changes needed?
We are able to specify default environment variables through the k8s plugin, but only those hard-coded into the configuration. Allowing default environment variables to be set via configMap or secret would allow automated systems in our environment to access secrets without embedding them in multiple locations.
What changes were proposed in this pull request?
Add ability to specify default configMap and secrets to mount on k8s plugin tasks.
How was this patch tested?
Unit tested, but local builds could not emulate the propeller build process currently.
Setup process
Screenshots
Check all the applicable boxes