-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
Add missed deprecations for cncf #20031
Add missed deprecations for cncf #20031
Conversation
Fixed the long lines that flake8 didn't like, but I'm not sure if import is something of concern (why is it not considering it top of the file if above is only license placeholder?) |
I would appreciate some advice on linter errors.... |
Maybe try with a newline before the first import? I'd play with it locally using pre-commit until you can get it happy, e.g. |
7211839
to
7025ead
Compare
Resolved both, it was because two comment blocks on top of file with |
Weird exception in deprecation warning validator. The line is already whitelisted in KNOWN_DEPRECATED_DIRECT_IMPORTS but still not enough for some reason. |
It actually detected a mistake :). It's about the stack_level and the fact that it is imported through another module. The warning is generated one level deeper than other warnings and stack_level must be set to 3, otherwise the generated warning will not show the origin of the import. As you can see here: https://github.com/apache/airflow/runs/4555876546?check_suite_focus=true#step:6:385 all the warnings are showing "importlib" as the import location but the new warning shows kubernetes_pod as the source:
|
Seems like now EKS block crashes with same |
Apparently the common "import" is using different stack levels in different import paths. This probably means that you need to make sure the warning is either generated with the same "stack level" of imports or raise the warning in different imports differently. |
The problem is that currently if user import the deprecate import now, they will see a wrong "origin" place of the import, so this is a "real" issue. |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. |
Co-authored-by: Jed Cunningham <[email protected]>
Co-authored-by: Jed Cunningham <[email protected]>
fed147d
to
f6f5a20
Compare
Should be sorted now. I had to sacrifice old references to imports. I feel deprecated class wasn't even intended to be imported by KPD anyway. I suspect users should have started using new |
This just adds deprecation warnings to backcompat classes for:
This was split from #19726 (comment)
@jedcunningham