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
Handling exception getting logs when pods finish success #39296
base: main
Are you sure you want to change the base?
Handling exception getting logs when pods finish success #39296
Conversation
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a test for this change?
8c0afca
to
6b9bb1d
Compare
82c40d8
to
628480a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look fine, few comments
def test_async_write_logs_handler_api_exception( | ||
self, mock_manager, mocked_hook, mock_extract_xcom | ||
): | ||
mock_manager.read_pod_logs.return_value = ApiException(status=404) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also add for non status of 404?
Also static checks need fixing. The best way would be to set up pre commit locally as we mention in our contributors guidelines |
32acac3
to
0d5987a
Compare
0d5987a
to
ce7c310
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good, one nit from me, rest is OK +1
@hussein-awala WDYT?
except ApiException as e: | ||
if e.status == 404: | ||
self.pod = None | ||
self.log.info( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make this warn
can you rebase and resolve conflicts? |
Closes: #39239
^ 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.