Skip to content

Conversation

@romsharon98
Copy link
Contributor

closes: #34154
cc: @SamWheating


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

@eladkal eladkal added this to the Airflow 2.9.0 milestone Apr 1, 2024
@eladkal eladkal requested a review from potiuk April 1, 2024 06:28
@eladkal eladkal added the type:bug-fix Changelog: Bug Fixes label Apr 1, 2024
Copy link
Contributor

@jscheffl jscheffl left a comment

Choose a reason for hiding this comment

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

Thanks for the fix, even did not know about this bug. Thanks also for the TODO; I assume there should be better options to parse the source code other than cutting by keywords.

@SamWheating
Copy link
Contributor

Thanks for reviving this old PR, I totally forgot about it but I'm glad to see that its back up for review 🙌

FWIW I still think that this is an imperfect solution (and anything which relies on manipulating code as a plain string probably will be), but it should at least fix the reported error without introducing new failures.

See additional notes in #34509.

@eladkal eladkal merged commit f1301da into apache:main Apr 1, 2024
@romsharon98
Copy link
Contributor Author

romsharon98 commented Apr 1, 2024

Thanks for the fix, even did not know about this bug. Thanks also for the TODO; I assume there should be better options to parse the source code other than cutting by keywords.

I only fixed a small test, the credits goes to @SamWheating

ephraimbuddy pushed a commit that referenced this pull request Apr 2, 2024
* Improve stability of remove_task_decorator function

* fix statics

* test

* remove test

---------

Co-authored-by: Sam Wheating <samwheating@gmail.com>
(cherry picked from commit f1301da)
ephraimbuddy pushed a commit that referenced this pull request Apr 2, 2024
* Improve stability of remove_task_decorator function

* fix statics

* test

* remove test

---------

Co-authored-by: Sam Wheating <samwheating@gmail.com>
(cherry picked from commit f1301da)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:bug-fix Changelog: Bug Fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

"@task.virtualenv" cannot appear in a comment inside a @task.virtualenv task

4 participants