You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2023/01/05 17:59:40 UTC
[GitHub] [airflow] csm10495 opened a new pull request, #28752: Allow setting the name for the base container
csm10495 opened a new pull request, #28752:
URL: https://github.com/apache/airflow/pull/28752
Some downstream machinery may require a specific name for the 'base' container.
This change should be backwards compatible since the default base container name is still base.. it can just now be changed to something besides base.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] csm10495 commented on a diff in pull request #28752: Allow setting the name for the base container and fix longstanding bug with running quick pods
Posted by GitBox <gi...@apache.org>.
csm10495 commented on code in PR #28752:
URL: https://github.com/apache/airflow/pull/28752#discussion_r1063075211
##########
airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -265,7 +265,7 @@ def consume_logs(*, since_time: DateTime | None = None, follow: bool = True) ->
time.sleep(1)
def await_container_completion(self, pod: V1Pod, container_name: str) -> None:
- while not self.container_is_running(pod=pod, container_name=container_name):
+ while self.container_is_running(pod=pod, container_name=container_name):
time.sleep(1)
Review Comment:
The problem is I can't get my new unit tests for base name with get_log=False without fixing this too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #28752: Allow setting the name for the base container and fix longstanding bug with running quick pods
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #28752:
URL: https://github.com/apache/airflow/pull/28752#discussion_r1063100091
##########
airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -265,7 +265,7 @@ def consume_logs(*, since_time: DateTime | None = None, follow: bool = True) ->
time.sleep(1)
def await_container_completion(self, pod: V1Pod, container_name: str) -> None:
- while not self.container_is_running(pod=pod, container_name=container_name):
+ while self.container_is_running(pod=pod, container_name=container_name):
time.sleep(1)
Review Comment:
Yeah let’s do that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] csm10495 commented on pull request #28752: Allow setting the name for the base container and fix longstanding bug with running quick pods
Posted by GitBox <gi...@apache.org>.
csm10495 commented on PR #28752:
URL: https://github.com/apache/airflow/pull/28752#issuecomment-1373956498
Closing in favor of #28771 and then redoing this after that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] csm10495 commented on pull request #28752: Allow setting the name for the base container and fix longstanding bug with running quick pods
Posted by GitBox <gi...@apache.org>.
csm10495 commented on PR #28752:
URL: https://github.com/apache/airflow/pull/28752#issuecomment-1372902051
This will also now fix #26796... since i kept hitting it in my tests :) @jedcunningham can you take a look when you get a chance?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #28752: Allow setting the name for the base container and fix longstanding bug with running quick pods
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #28752:
URL: https://github.com/apache/airflow/pull/28752#discussion_r1063069166
##########
airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -265,7 +265,7 @@ def consume_logs(*, since_time: DateTime | None = None, follow: bool = True) ->
time.sleep(1)
def await_container_completion(self, pod: V1Pod, container_name: str) -> None:
- while not self.container_is_running(pod=pod, container_name=container_name):
+ while self.container_is_running(pod=pod, container_name=container_name):
time.sleep(1)
Review Comment:
While this looks like a valid fix, it is logically unrelated to the `base_container_name` change. Could you split it into another PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] csm10495 commented on a diff in pull request #28752: Allow setting the name for the base container and fix longstanding bug with running quick pods
Posted by GitBox <gi...@apache.org>.
csm10495 commented on code in PR #28752:
URL: https://github.com/apache/airflow/pull/28752#discussion_r1063075211
##########
airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -265,7 +265,7 @@ def consume_logs(*, since_time: DateTime | None = None, follow: bool = True) ->
time.sleep(1)
def await_container_completion(self, pod: V1Pod, container_name: str) -> None:
- while not self.container_is_running(pod=pod, container_name=container_name):
+ while self.container_is_running(pod=pod, container_name=container_name):
time.sleep(1)
Review Comment:
The problem is I can't get my new unit tests for base name with get_log=False to pass without fixing this too.
I guess I can split to do this fix first then that one after if you'd prefer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] csm10495 closed pull request #28752: Allow setting the name for the base container and fix longstanding bug with running quick pods
Posted by GitBox <gi...@apache.org>.
csm10495 closed pull request #28752: Allow setting the name for the base container and fix longstanding bug with running quick pods
URL: https://github.com/apache/airflow/pull/28752
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] csm10495 commented on pull request #28752: Allow setting the name for the base container
Posted by GitBox <gi...@apache.org>.
csm10495 commented on PR #28752:
URL: https://github.com/apache/airflow/pull/28752#issuecomment-1372613163
> Would be nice to have unit tests which cover new functionality for avoid regression
Yep.. they're on the way :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] csm10495 commented on a diff in pull request #28752: Allow setting the name for the base container and fix longstanding bug with running quick pods
Posted by GitBox <gi...@apache.org>.
csm10495 commented on code in PR #28752:
URL: https://github.com/apache/airflow/pull/28752#discussion_r1063104726
##########
airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -265,7 +265,7 @@ def consume_logs(*, since_time: DateTime | None = None, follow: bool = True) ->
time.sleep(1)
def await_container_completion(self, pod: V1Pod, container_name: str) -> None:
- while not self.container_is_running(pod=pod, container_name=container_name):
+ while self.container_is_running(pod=pod, container_name=container_name):
time.sleep(1)
Review Comment:
ack. will split em tomorrow.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on pull request #28752: Allow setting the name for the base container
Posted by GitBox <gi...@apache.org>.
Taragolis commented on PR #28752:
URL: https://github.com/apache/airflow/pull/28752#issuecomment-1372612235
Would be nice to have unit tests which cover new functionality for avoid regression
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] csm10495 commented on a diff in pull request #28752: Allow setting the name for the base container and fix longstanding bug with running quick pods
Posted by GitBox <gi...@apache.org>.
csm10495 commented on code in PR #28752:
URL: https://github.com/apache/airflow/pull/28752#discussion_r1063075211
##########
airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -265,7 +265,7 @@ def consume_logs(*, since_time: DateTime | None = None, follow: bool = True) ->
time.sleep(1)
def await_container_completion(self, pod: V1Pod, container_name: str) -> None:
- while not self.container_is_running(pod=pod, container_name=container_name):
+ while self.container_is_running(pod=pod, container_name=container_name):
time.sleep(1)
Review Comment:
The problem is I can't get my new unit tests for base name with get_log=False to pass without fixing this too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] csm10495 commented on a diff in pull request #28752: Allow setting the name for the base container and fix longstanding bug with running quick pods
Posted by GitBox <gi...@apache.org>.
csm10495 commented on code in PR #28752:
URL: https://github.com/apache/airflow/pull/28752#discussion_r1063652449
##########
airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -265,7 +265,7 @@ def consume_logs(*, since_time: DateTime | None = None, follow: bool = True) ->
time.sleep(1)
def await_container_completion(self, pod: V1Pod, container_name: str) -> None:
- while not self.container_is_running(pod=pod, container_name=container_name):
+ while self.container_is_running(pod=pod, container_name=container_name):
time.sleep(1)
Review Comment:
@uranusjr i've split this part into: https://github.com/apache/airflow/pull/28771 ... after that merges, i'll recreate this pr.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org