Skip to content
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

Allow xcom sidecar container image to be configurable #26766

Merged
merged 61 commits into from
Nov 9, 2022

Conversation

bdsoha
Copy link
Contributor

@bdsoha bdsoha commented Sep 29, 2022

Closes #10605.

Adding a configurable image value to the KubernetesHook which is used by PodDefaults.SIDECAR_CONTAINER.image will allow both:

  1. Images hosted on private repositories.
  2. Overriding the default alpine:latest image with a specific tag.

@boring-cyborg boring-cyborg bot added provider:cncf-kubernetes Kubernetes provider related issues area:providers labels Sep 29, 2022
@bdsoha bdsoha changed the title Add sidecar container image to configs Allow sidecar container image to be configurable Sep 29, 2022
This should be valid until the code is actually removed from the codebase.
@bdsoha bdsoha requested a review from dstandish as a code owner September 29, 2022 17:32
@bdsoha bdsoha changed the title Allow sidecar container image to be configurable Allow xcom sidecar container image to be configurable Sep 29, 2022
@bdsoha bdsoha force-pushed the feature/sidecar_container branch from a82cb8d to cf0c9a6 Compare September 29, 2022 23:40
@bdsoha bdsoha marked this pull request as draft September 30, 2022 08:04
@bdsoha bdsoha marked this pull request as ready for review September 30, 2022 08:51
@eladkal eladkal requested a review from jedcunningham October 25, 2022 04:15
@bdsoha bdsoha requested review from XD-DENG and dstandish and removed request for dstandish, jedcunningham and XD-DENG November 2, 2022 19:47
@eladkal eladkal requested a review from dstandish November 6, 2022 06:25
Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

LGTM. But we need to wait for @dstandish's re-review.

@bdsoha
Copy link
Contributor Author

bdsoha commented Nov 8, 2022

@dstandish @XD-DENG Any we can get this merged? Thanks in advance.

@dstandish
Copy link
Contributor

ok lemme look

Copy link
Contributor

@dstandish dstandish left a comment

Choose a reason for hiding this comment

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

mainly one docs reqeust

airflow/providers/cncf/kubernetes/hooks/kubernetes.py Outdated Show resolved Hide resolved
airflow/providers/cncf/kubernetes/hooks/kubernetes.py Outdated Show resolved Hide resolved
@bdsoha bdsoha requested review from dstandish and removed request for XD-DENG November 9, 2022 05:56
@dstandish dstandish merged commit aefadb8 into apache:main Nov 9, 2022
@bdsoha bdsoha deleted the feature/sidecar_container branch November 9, 2022 06:22
dstandish added a commit to astronomer/airflow that referenced this pull request May 12, 2023
We probably should not be configuring xcom settings from the hook in this way but for better or worse we've done it, first in apache#26766 and again in apache#28125.

The problem is that other operators derived from KPO, e.g. GKEStartPodOperator, may use a different hook that doesn't have this xcom-specific methods.  So KPO needs to tolerate that.
@raycarter
Copy link

Hello, is this PR in any version released? I can't find the changes in the current version 2.6.3 or in the main-branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers provider:cncf-kubernetes Kubernetes provider related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use private docker repository with K8S operator and XCOM sidecar container
6 participants