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

Do not convert boolean values to string in deep_string_coerce function #25394

Merged
merged 12 commits into from
Aug 4, 2022
Merged

Do not convert boolean values to string in deep_string_coerce function #25394

merged 12 commits into from
Aug 4, 2022

Conversation

jgr-trackunit
Copy link
Contributor

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

  • Unittests have been updated, unit tests have been run with Passed result
  • Docstring for deep_string_coerce function has been updated
  • Command breeze static-checks --all passed

@potiuk @aru-trackunit

@boring-cyborg
Copy link

boring-cyborg bot commented Jul 29, 2022

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)
Here are some useful points:

  • Pay attention to the quality of your code (flake8, mypy and type annotations). Our pre-commits will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
  • Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: dev@airflow.apache.org
    Slack: https://s.apache.org/airflow-slack

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

LGTM. @alexott WDYT?

Copy link
Contributor

@alexott alexott left a 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

@uranusjr
Copy link
Member

uranusjr commented Aug 1, 2022

The logic is correct, but the function should not be named deep_string_coerce.

@jgr-trackunit
Copy link
Contributor Author

HI @potiuk @uranusjr

So shall I rename that function? Should it be the part of another PR? Can I merge it?

@potiuk
Copy link
Member

potiuk commented Aug 2, 2022

HI @potiuk @uranusjr
So shall I rename that function? Should it be the part of another PR? Can I merge it?

Yes. rename it please. Why do you ask? Is there any doubt about it ? I thinkt it was clear and justified ask.

@jgr-trackunit
Copy link
Contributor Author

jgr-trackunit commented Aug 2, 2022

HI @potiuk @uranusjr
So shall I rename that function? Should it be the part of another PR? Can I merge it?

Yes. rename it please. Why do you ask? Is there any doubt about it ? I thinkt it was clear and justified ask.

I was just not sure if we want to change that function name in this PR. :)

I would like to propose new name: either normalise_json_content or normalise_content, what do you think @potiuk @uranusjr? Would be that name consistent enough?

@potiuk
Copy link
Member

potiuk commented Aug 2, 2022

HI @potiuk @uranusjr
So shall I rename that function? Should it be the part of another PR? Can I merge it?

Yes. rename it please. Why do you ask? Is there any doubt about it ? I thinkt it was clear and justified ask.

I was just not sure if we want to change that function name in this PR. :)

I would like to propose new name: either normalise_json_content or normalise_content, what do you think @potiuk @uranusjr? Would be that name consistent enough?

both are better :). Pick one you like :D

@jgr-trackunit
Copy link
Contributor Author

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?

@potiuk
Copy link
Member

potiuk commented Aug 3, 2022

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 breeze tool helps with that: https://github.com/apache/airflow/blob/main/BREEZE.rst#preparing-provider-packages

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 demands have exactly 0 worth here (unless they are polite asks).

BTW. I am the release manager of providers.

@potiuk
Copy link
Member

potiuk commented Aug 3, 2022

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

@jgr-trackunit
Copy link
Contributor Author

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.

@potiuk
Copy link
Member

potiuk commented Aug 3, 2022

So apologies one more time for that, It was not the goal of that message.

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.

I see that chapter is missing: https://github.com/apache/airflow/blob/main/README.md#release-process-for-providers (but I read it ealier)

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 :)

@uranusjr
Copy link
Member

uranusjr commented Aug 4, 2022

tests/providers/databricks/operators/test_databricks.py::TestDatabricksRunNowOperator::test_exec_failure_with_message:
AttributeError: module 'airflow.providers.databricks.utils.databricks' has no attribute 'deep_string_coerce'

@jgr-trackunit
Copy link
Contributor Author

tests/providers/databricks/operators/test_databricks.py::TestDatabricksRunNowOperator::test_exec_failure_with_message:
AttributeError: module 'airflow.providers.databricks.utils.databricks' has no attribute 'deep_string_coerce'

Fixed

@uranusjr
Copy link
Member

uranusjr commented Aug 4, 2022

The spell checker is American

File path: apache-airflow-providers-databricks/_api/airflow/providers/databricks/utils/databricks/index.rst
Incorrect Spelling: 'Normalise'
Line with Error: 'Normalise content or all values of content if it is a dict to a string. The'
Line Number: 23

@potiuk potiuk merged commit 0255a0a into apache:main Aug 4, 2022
@boring-cyborg
Copy link

boring-cyborg bot commented Aug 4, 2022

Awesome work, congrats on your first merged pull request!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

enable_elastic_disk property incorrectly mapped when making a request to Databricks
4 participants