Skip to content

Conversation

@pankajastro
Copy link
Member

closes: #38105


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborg boring-cyborg bot added area:providers provider:cncf-kubernetes Kubernetes (k8s) provider related issues labels Mar 18, 2024
@pankajastro pankajastro marked this pull request as ready for review March 18, 2024 10:28
Copy link
Member

@hussein-awala hussein-awala left a comment

Choose a reason for hiding this comment

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

Why do we need to filter empty log lines? From the reported issue, I see we have a problem with None, but IMHO we have to keep empty lines because the user may want to log them. WDYT?

(requested a change to avoid merging it before discussing the solution)

@pankajastro
Copy link
Member Author

pankajastro commented Mar 20, 2024

Why do we need to filter empty log lines? From the reported issue, I see we have a problem with None, but IMHO we have to keep empty lines because the user may want to log them. WDYT?

(requested a change to avoid merging it before discussing the solution)

I'm ok to change the check to None only.
2 logs I find a bit weird

  1. INFO - [base] None i.e logging None. coming from pod_manager.py and in this case this log is unintentional
  2. Container logs: i.e logging empty. coming from pod.py this also look unintentional

WDYT if I'll change pod_manager.py check to

if message_to_log is not None:

i.e just avoid printing None
And keep pod.py as it is in current form?

@pankajastro
Copy link
Member Author

Hey @hussein-awala, would you like to review this one again? Thank you!

@eladkal eladkal requested a review from hussein-awala April 2, 2024 04:45
@anteverse
Copy link
Contributor

Any updates on this? The last version seems to answer all problems and all demands.

@pankajastro pankajastro merged commit 43919c2 into apache:main Apr 15, 2024
@pankajastro pankajastro deleted the avoid_empty_log branch April 15, 2024 15:22
@anteverse
Copy link
Contributor

Thanks a lot!

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 (k8s) provider related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Extra "None" logs from Kubernetes pod while running a KubernetesPodOperator

4 participants