-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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
Do not convert boolean values to string in deep_string_coerce function #25394
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 Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.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.
LGTM. @alexott WDYT?
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.
Patch looks good to me. I always had a problem with that function because it’s more a workaround around the render_template function. If render_template can handle non-string types gracefully, then we don’t need this function at all.
p.s. in my other projects I’ve implemented templates, but it was other way around, string templates were converted into non-string types if variables were of these types
The logic is correct, but the function should not be named |
I was just not sure if we want to change that function name in this PR. :) I would like to propose new name: either |
both are better :). Pick one you like :D |
Who can merge it now? What about the release process? I'm interested to have this changes released ASAP, so I can help somehow with that, ie. testing it in real env. Could you tell me who is the release manager? |
It's quite rude way of asking for things. I would expect you politely ask rather than demand things. You have to be patient (and also empathetic to the process we have and others). This is a very basic thing when you interface with open-source software that you should learn. We release providers roughly once a month. This is clearly described in the docuementation. There is no way this can be sped up because "you" need it. If there is big problem for the community impacting a lot of people, we might do ad-hoc releases but this is not at all needed here. The release process is described here: https://github.com/apache/airflow/blob/main/README.md#release-process-for-providers And if you REALLY want to do things ASAP there is no problem for you to build and use the provider with your changes built yourself. Building provider packages is easy and You have to remember that there is nothing like ASAP in open-source world. You (and your company) paid exactly 0 USD for the free software (that many people deliver for free). So your BTW. I am the release manager of providers. |
And yes. We will ask you to test it when we release it together with other providers. This is also part of the standard process - you can see how it was done before: https://github.com/apache/airflow/issues?q=is%3Aissue+status+of+testing+is%3Aclosed+label%3A%22testing+status%22 |
Really sorry for my previous message I didn't want to offend yourself and I didn't demand anything. As well I'm not trying to force you to release it now. So apologies one more time for that, It was not the goal of that message. I see that chapter is missing: https://github.com/apache/airflow/blob/main/README.md#release-process-for-providers (but I read it ealier) Thanks for the 2nd link. |
No worries. Apologies for harsh tone. It's a bit of being sensitive after some "real" demands I saw before. Thanks for being sensitive and receptive to it.
It's there, just GitHub having problems with scrolling down to it's own anchors (Sometimes) - seen that many times. Usually reloading it the second time works :) |
|
Fixed |
The spell checker is American
|
Awesome work, congrats on your first merged pull request! |
When we send request to Databricks with quoted bool values like:
"False"
and"True"
Databricks interprets them as a strings which is undesired behaviour. In that case we need to send requests with real boolean values.closes: #25232
related:#25232
Passed
resultdeep_string_coerce
function has been updatedbreeze static-checks --all
passed@potiuk @aru-trackunit