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

Replace caplog with patching log property in k8s tests #46273

Merged
merged 1 commit into from
Jan 30, 2025

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Jan 30, 2025

Since caplog is very vulnerable to side effects we remove it from k8s tests and replace it with patching the log property of the K8SHook. This should not be vulnerable to side-effects.


^ 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.

@bugraoz93
Copy link
Collaborator

Look great! Thanks!
Should we aim to maybe scan and plan to remove caplog pieces other than cases where it is strictly necessary? Or do we need a lazy consensus to decide removing caplog pieces? Also, including a CI check to prevent to be included more caplog test without mocking if decisionis more towards to this approach?

@potiuk
Copy link
Member Author

potiuk commented Jan 30, 2025

Should we aim to maybe scan and plan to remove caplog pieces other than cases where it is strictly necessary?

Yes

Or do we need a lazy consensus to decide removing caplog pieces?

Might be worth doing to get people aware/engaged.

Also, including a CI check to prevent to be included more caplog test without mocking if decisionis more towards to this approach?

Absolutely.

Go ahead with that :)

Since caplog is very vulnerable to side effects we remove it from
k8s tests and replace it with patching the log property of the
K8SHook. This should not be vulnerable to side-effects.
@potiuk potiuk force-pushed the replace-caplog-with-log-mocking branch from 6dbfbf9 to c0acea6 Compare January 30, 2025 02:00
@potiuk potiuk merged commit 3187141 into apache:main Jan 30, 2025
17 checks passed
@potiuk potiuk deleted the replace-caplog-with-log-mocking branch January 30, 2025 02:04
got686-yandex pushed a commit to got686-yandex/airflow that referenced this pull request Jan 30, 2025
Since caplog is very vulnerable to side effects we remove it from
k8s tests and replace it with patching the log property of the
K8SHook. This should not be vulnerable to side-effects.
dabla pushed a commit to dabla/airflow that referenced this pull request Jan 30, 2025
Since caplog is very vulnerable to side effects we remove it from
k8s tests and replace it with patching the log property of the
K8SHook. This should not be vulnerable to side-effects.
ambika-garg pushed a commit to ambika-garg/airflow that referenced this pull request Jan 30, 2025
Since caplog is very vulnerable to side effects we remove it from
k8s tests and replace it with patching the log property of the
K8SHook. This should not be vulnerable to side-effects.
@bugraoz93
Copy link
Collaborator

Should we aim to maybe scan and plan to remove caplog pieces other than cases where it is strictly necessary?

Yes

Or do we need a lazy consensus to decide removing caplog pieces?

Might be worth doing to get people aware/engaged.

Also, including a CI check to prevent to be included more caplog test without mocking if decisionis more towards to this approach?

Absolutely.

Go ahead with that :)

Amazing! I will make the engagement and will create follow-ups :) This was one of the things I always wanted to support and wished to make it happen 🥳

@potiuk
Copy link
Member Author

potiuk commented Feb 1, 2025

Amazing! I will make the engagement and will create follow-ups :) This was one of the things I always wanted to support and wished to make it happen 🥳

👀 👀 👀 👀 👀

niklasr22 pushed a commit to niklasr22/airflow that referenced this pull request Feb 8, 2025
Since caplog is very vulnerable to side effects we remove it from
k8s tests and replace it with patching the log property of the
K8SHook. This should not be vulnerable to side-effects.
ambika-garg pushed a commit to ambika-garg/airflow that referenced this pull request Feb 17, 2025
Since caplog is very vulnerable to side effects we remove it from
k8s tests and replace it with patching the log property of the
K8SHook. This should not be vulnerable to side-effects.
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.

3 participants