Prevent assignment of non JSON serializable values to DagRun.conf dict#35096
Conversation
ConfDict class added, conf attribute updated
Error description fixed
back to previous annotation
|
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 Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
|
jscheffl
left a comment
There was a problem hiding this comment.
This looks good. Thanks for the contribution.
I was thinging (but have no strong opinion) as whether it makes sense to extract this to a utility, but keeping it here might be acceptable.
Can you please add a unit test for the new code which validates a positive and negative case as well?
yes unit test is required here |
conf fixed
Could you, please, advise me where I should add this test? This module? |
If you implement code in |
Exception description fixed
Error fixed
Test added
|
There are some (simple) static code check problems. Before making an approval review I'd like see them fixed. But "okay" from my side. I am not an expert in sqlachemy so I am a bit afraid from the previous review comment from @uranusjr - can you maybe double check this? Of course we need to prevent a migration for this. |
hybrid_property for column conf has been added
declared_attr added
|
I’ve been mistaken about hybrid_property, cause it doesn’t work correctly with mypy, should be using declared_attr instead. |
|
Just small glitches in static code checks. If you remove them I assume it is good to get it merged. |
Docstring Content Issue fixed
Codestyle fixed
|
Still no luck with static checks, can you take a look here? Actually it just seems you need to run it once and commit the changed applied from ruff: https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst |
trailing whitespace removed
line break added
|
@jscheffl |
|
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
…conf dict (apache#35096)" This reverts commit 84c40a7.
Closes #35095