Skip to content

backport for add_xcom_sidecar #11478

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.

Sign up for GitHub

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

Jump to bottom
Merged
dimberman merged 5 commits into from
Oct 14, 2020
Merged

backport for add_xcom_sidecar #11478

dimberman merged 5 commits into from
Oct 14, 2020

Conversation

@dimberman
Copy link
Contributor

@dimberman dimberman commented Oct 12, 2020

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

@dimberman dimberman requested a review from potiuk October 12, 2020 20:26
@dimberman dimberman mentioned this pull request Oct 12, 2020
Closed
@dimberman dimberman linked an issue Oct 12, 2020 that may be closed by this pull request
Kubernetes - related backports needs some fixes for xcom cases #11298
Closed
@dimberman dimberman requested review from kaxil and mik-laj October 12, 2020 20:28
@dimberman
Copy link
Contributor Author

dimberman commented Oct 12, 2020

cc: @michalmisiewicz

@dimberman dimberman removed the request for review from kaxil October 12, 2020 21:07
@mik-laj
Copy link
Member

mik-laj commented Oct 12, 2020

@michalslowikowski00 Can you look at it?

kaxil
kaxil reviewed Oct 13, 2020
View reviewed changes
@dimberman dimberman force-pushed the backport-refactor-k8s-pod-operator branch from 0056683 to 757e48a Compare October 13, 2020 03:13
@michalslowikowski00
Copy link
Contributor

michalslowikowski00 commented Oct 13, 2020 •
edited
Loading

Is it a fix for 11298?

@mik-laj
Copy link
Member

mik-laj commented Oct 13, 2020

@michalslowikowski00 Yes. Can you test it?

@dimberman
Copy link
Contributor Author

dimberman commented Oct 13, 2020

@michalslowikowski00 if you have a chance to test it I'd appreciate it, can merge once I hear back :)

@michalslowikowski00
Copy link
Contributor

michalslowikowski00 commented Oct 13, 2020

Yes, I will test it.

@michalslowikowski00
Copy link
Contributor

michalslowikowski00 commented Oct 14, 2020

Hey @dimberman.
I just tested the fix and it doesn't work. Sorry. :(
I will try to figure it out.

@mik-laj
Copy link
Member

mik-laj commented Oct 14, 2020

Changing add_xcom_sidecar to add_sidecar works, but the Bowler code does not make expected change. I together with @michalslowikowski00 made these changes manually to test it.

@dimberman
Copy link
Contributor Author

dimberman commented Oct 14, 2020

@mik-laj Any idea why bowler isn't making the change?

@dimberman
Copy link
Contributor Author

dimberman commented Oct 14, 2020

Maybe I need to remove the filter?

potiuk
potiuk reviewed Oct 14, 2020
View reviewed changes
provider_packages/refactor_provider_packages.py Outdated

(
self.qry.
select_class("airflow.kubernetes.pod_generator.PodGenerator").
Copy link
Member

@potiuk potiuk Oct 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe select_class works only on name not whole class including package/module. See some examples below. If you want to perform an operation on specific class in specific module, you can provide filename filters I think.

Copy link
Member

@potiuk potiuk Oct 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dimberman

Copy link
Member

@potiuk potiuk Oct 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW. you can easily test Bowler and debug it. You just need to run this as python code and add --debug, see below comment in main (but it is a lot slower and it's better to comment out all other filters only leaving the one you are working on.

@dimberman
Copy link
Contributor Author

dimberman commented Oct 14, 2020

@potiuk @mik-laj I got it working

(airflow) new-features  (backport-refactor-k8s-pod-operator)  (⎈ |gke_astronomer-cloud-dev-236021_us-east4-a_dev-cluster|airflow-2-test) $
python provider_packages/refactor_provider_packages.py --debug
--- ./airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py
+++ ./airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py
@@ -407,7 +407,7 @@
         for secret in self.secrets:
             pod = secret.attach_to_pod(pod)
         if self.do_xcom_push:
-            pod = PodGenerator.add_xcom_sidecar(pod)
+            pod = PodGenerator.add_sidecar(pod)
         return pod

@dimberman dimberman merged commit 7b7cc3c into apache:master Oct 14, 2020
@dimberman dimberman deleted the backport-refactor-k8s-pod-operator branch October 14, 2020 19:49
@mik-laj
Copy link
Member

mik-laj commented Oct 14, 2020

🎉 Thanks for this PR. It's just a few lines of code, but very valuable. Many users wait until they can use this operator.

@potiuk
Copy link
Member

potiuk commented Oct 14, 2020

🎉 !

@michalslowikowski00
Copy link
Contributor

michalslowikowski00 commented Oct 15, 2020

Great! 🚀

@potiuk potiuk mentioned this pull request Oct 16, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@potiuk potiuk potiuk left review comments

@kaxil kaxil kaxil approved these changes

@mik-laj mik-laj mik-laj approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Kubernetes - related backports needs some fixes for xcom cases

5 participants

@dimberman @mik-laj @michalslowikowski00 @potiuk @kaxil