[AIRFLOW-4008] add envFrom for Kubernetes Executor by davlum · Pull Request #4952 · apache/airflow

davlum · 2019-03-20T15:45:12Z

Make sure you have checked all steps below.

Jira

My PR addresses the following Airflow Jira

Description

Here are some details about my PR, including screenshots of any UI changes:

This PR enables workers brought up by the KubernetesExecutor to use the envFrom feature to set environment variables.

Tests

My PR adds the following unit tests.

Commits

My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters (not including Jira issue reference)
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

Documentation

In case of new functionality, my PR adds documentation that describes how to use it.
- When adding new operators/hooks/sensors, the autoclass documentation generation needs to be added.
- All the public functions and the classes in the PR contain docstrings that explain what it does
- If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

Code Quality

Passes flake8

…AIRFLOW-4008/k8s-exec-envfrom-configmap

dimberman

+1 on this feature. Could you please add an integration test?

ashb · 2019-03-21T09:56:08Z

Please prefix your commits with [AIRFLOW-4008]

There is some overlap between this PR and #4772 - can you and @galuszkak work out how do get both these in without duplicating code/what makes sense where etc.

galuszkak

Some feedback to align this with #4772 . I'm working on my PR for 3 weeks already, there was a lot of code reviews there, so hopefully, this can help catch changes that I made because of different requests in CR.

galuszkak · 2019-03-21T12:17:51Z

+        self.kube_env_from_configmap_ref = configuration.get(self.kubernetes_section,
+                                                             'env_from_configmap_ref')
+        self.kube_env_from_secret_ref = configuration.get(self.kubernetes_section,
+                                                          'env_from_secret_ref')


To be consistent with #4772 I would suggest using self.kube_secrets and add self.kube_configmaps .

davlum · 2019-03-21T14:47:55Z

I think the simplest move here would be to get @galuszkak's PR merged first, and I can just work off the changes that make it into master. It's true that the PodRequestFactory was previously entirely untested, so I can see why @galuszkak is reluctant to have that responsibility dumped onto #4772. However, unittesting PodRequestFactory is something I've started doing in this PR and could make more comprehensive. If #4772 gets merged I can add the unittests, timeline being ideally before the end of the week. @ashb Would it be possible to get this in before 1.10.3?

XD-DENG

Docstring style change is needed.

galuszkak · 2019-03-27T12:32:32Z

@davlum my changes were merged, so I believe You can start working back on this PR.

ashb

Partial review (on mobile)

Nice improvement to the tests.

I wonder if we can reduce code in tests some how - want to look at that on a bigger screen

davlum · 2019-03-28T18:39:27Z

Let me know if you think some of the tests are redundant. I just refactored the Secrets object to be a bit more intuitive. I find optionally setting attributes of an object to be very difficult to reason about.

…AIRFLOW-4008/k8s-exec-envfrom-configmap

codecov-io · 2019-03-31T03:14:51Z

Codecov Report

Merging #4952 into master will increase coverage by 0.11%.
The diff coverage is 95.45%.

@@            Coverage Diff             @@
##           master    #4952      +/-   ##
==========================================
+ Coverage   75.99%   76.11%   +0.11%     
==========================================
  Files         461      461              
  Lines       29968    29979      +11     
==========================================
+ Hits        22774    22818      +44     
+ Misses       7194     7161      -33

Impacted Files	Coverage Δ
airflow/contrib/kubernetes/pod.py	`100% <ø> (ø)`	⬆️
...etes_request_factory/kubernetes_request_factory.py	`99.03% <100%> (+26.92%)`	⬆️
airflow/contrib/kubernetes/worker_configuration.py	`95.72% <100%> (+2.09%)`	⬆️
airflow/contrib/executors/kubernetes_executor.py	`63.17% <100%> (+0.2%)`	⬆️
airflow/contrib/kubernetes/secret.py	`93.33% <88.88%> (+24.1%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8751133...6e0068f. Read the comment docs.

* deepcopy objects

dimberman · 2019-04-02T18:12:09Z

@ashb @XD-DENG this LGTM, do you guys have any remaining issues? If not I can merge.

XD-DENG · 2019-04-02T23:44:48Z

Thanks @dimberman . No other comment/issue to me.

davlum force-pushed the feat/AIRFLOW-4008/k8s-exec-envfrom-configmap branch 3 times, most recently from 678fbd1 to cdbf418 Compare March 20, 2019 18:56

feat/AIRFLOW-4008/k8s-executor-env-from

3e7caf1

davlum force-pushed the feat/AIRFLOW-4008/k8s-exec-envfrom-configmap branch from cdbf418 to 3e7caf1 Compare March 20, 2019 19:46

Merge branch 'master' of https://github.com/apache/airflow into feat/…

c5fdf13

…AIRFLOW-4008/k8s-exec-envfrom-configmap

dimberman requested changes Mar 20, 2019

View reviewed changes

Comment thread tests/contrib/kubernetes/kubernetes_request_factory/test_pod_request_factory.py

ashb added the k8s label Mar 21, 2019

ashb reviewed Mar 21, 2019

View reviewed changes

Comment thread airflow/contrib/kubernetes/pod.py Outdated

galuszkak suggested changes Mar 21, 2019

View reviewed changes

davlum changed the title ~~feat/AIRFLOW-4008/k8s-executor-env-from~~ [AIRFLOW-4008] add envFrom for Kubernetes Executor Mar 21, 2019

XD-DENG requested changes Mar 22, 2019

View reviewed changes

Comment thread airflow/contrib/kubernetes/pod.py Outdated

ashb mentioned this pull request Mar 25, 2019

[AIRFLOW-3937] KubernetesPodOperator support for envFrom configMapRef… #4772

Merged

5 tasks

[AIRFLOW-4008] merge with apache#4772 and add tests

f274415

ashb reviewed Mar 28, 2019

View reviewed changes

Comment thread tests/contrib/kubernetes/kubernetes_request_factory/test_kubernetes_request_factory.py Outdated

Comment thread tests/contrib/kubernetes/kubernetes_request_factory/test_kubernetes_request_factory.py Outdated

davlum force-pushed the feat/AIRFLOW-4008/k8s-exec-envfrom-configmap branch 2 times, most recently from ac451a5 to 665bbc0 Compare March 28, 2019 19:36

ashb reviewed Mar 29, 2019

View reviewed changes

Comment thread tests/contrib/kubernetes/kubernetes_request_factory/test_kubernetes_request_factory.py Outdated

ashb reviewed Mar 29, 2019

View reviewed changes

[AIRFLOW-4008] refactor Secrets

93384db

davlum force-pushed the feat/AIRFLOW-4008/k8s-exec-envfrom-configmap branch from 665bbc0 to 93384db Compare March 29, 2019 15:35

David Lum added 2 commits March 29, 2019 12:24

[AIRFLOW-4008] fix doc errors

26f8c63

Merge branch 'master' of https://github.com/apache/airflow into feat/…

ee0e808

…AIRFLOW-4008/k8s-exec-envfrom-configmap

davlum force-pushed the feat/AIRFLOW-4008/k8s-exec-envfrom-configmap branch 2 times, most recently from 1026bac to 4c590ba Compare March 29, 2019 19:43

davlum force-pushed the feat/AIRFLOW-4008/k8s-exec-envfrom-configmap branch 7 times, most recently from b6b10e4 to 9191d80 Compare March 31, 2019 02:28

davlum force-pushed the feat/AIRFLOW-4008/k8s-exec-envfrom-configmap branch from 9191d80 to 5a7b0aa Compare March 31, 2019 14:21

[AIRFLOW-4008] don't initialize object, use static methods

6e0068f

* deepcopy objects

davlum force-pushed the feat/AIRFLOW-4008/k8s-exec-envfrom-configmap branch from 5a7b0aa to 6e0068f Compare April 1, 2019 18:13

dimberman approved these changes Apr 2, 2019

View reviewed changes

XD-DENG approved these changes Apr 2, 2019

View reviewed changes

ashb approved these changes Apr 3, 2019

View reviewed changes

ashb merged commit e72ff0a into apache:master Apr 3, 2019

ashb pushed a commit that referenced this pull request Apr 3, 2019

[AIRFLOW-4008] add envFrom for Kubernetes Executor (#4952)

4a05201

ashb mentioned this pull request Apr 3, 2019

[AIRFLOW-3274] Add run_as_user and fs_group options for Kubernetes #4648

Merged

6 tasks

cthenderson pushed a commit to cthenderson/apache-airflow that referenced this pull request Apr 16, 2019

[AIRFLOW-4008] add envFrom for Kubernetes Executor (apache#4952)

418864c

andriisoldatenko pushed a commit to andriisoldatenko/airflow that referenced this pull request Jul 26, 2019

[AIRFLOW-4008] add envFrom for Kubernetes Executor (apache#4952)

7f45c51

wmorris75 pushed a commit to modmed-external/incubator-airflow that referenced this pull request Jul 29, 2019

[AIRFLOW-4008] add envFrom for Kubernetes Executor (apache#4952)

0f9e7f7

davlum deleted the feat/AIRFLOW-4008/k8s-exec-envfrom-configmap branch October 3, 2019 19:03

jkleckner mentioned this pull request Jan 30, 2020

Support for envFrom to set environment variables from a configmap kubeflow/spark-operator#785

Closed

Conversation

davlum commented Mar 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Jira

Description

Tests

Commits

Documentation

Code Quality

Uh oh!

dimberman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ashb commented Mar 21, 2019

Uh oh!

galuszkak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

galuszkak Mar 21, 2019

Choose a reason for hiding this comment

Uh oh!

davlum commented Mar 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

XD-DENG left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

galuszkak commented Mar 27, 2019

Uh oh!

ashb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

davlum commented Mar 28, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-io commented Mar 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dimberman commented Apr 2, 2019

Uh oh!

XD-DENG commented Apr 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

davlum commented Mar 20, 2019 •

edited

Loading

davlum commented Mar 21, 2019 •

edited

Loading

codecov-io commented Mar 31, 2019 •

edited

Loading