You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/07/29 09:48:20 UTC

[GitHub] [airflow] jgr-trackunit opened a new pull request, #25394: Do not convert boolean values to string in deep_string_coerce function

jgr-trackunit opened a new pull request, #25394:
URL: https://github.com/apache/airflow/pull/25394

   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](https://github.com/apache/airflow/issues/25232)
   related:[#25232](https://github.com/apache/airflow/issues/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 
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] jgr-trackunit commented on pull request #25394: Do not convert boolean values to string in deep_string_coerce function

Posted by GitBox <gi...@apache.org>.
jgr-trackunit commented on PR #25394:
URL: https://github.com/apache/airflow/pull/25394#issuecomment-1202423115

   > > 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_json_content`, what do you think @potiuk @uranusjr? Would be that name consistent enough?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] uranusjr commented on pull request #25394: Do not convert boolean values to string in deep_string_coerce function

Posted by GitBox <gi...@apache.org>.
uranusjr commented on PR #25394:
URL: https://github.com/apache/airflow/pull/25394#issuecomment-1204753518

   ```
   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'
   ```


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] uranusjr commented on pull request #25394: Do not convert boolean values to string in deep_string_coerce function

Posted by GitBox <gi...@apache.org>.
uranusjr commented on PR #25394:
URL: https://github.com/apache/airflow/pull/25394#issuecomment-1200693089

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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] jgr-trackunit commented on pull request #25394: Do not convert boolean values to string in deep_string_coerce function

Posted by GitBox <gi...@apache.org>.
jgr-trackunit commented on PR #25394:
URL: https://github.com/apache/airflow/pull/25394#issuecomment-1202204175

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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk commented on pull request #25394: Do not convert boolean values to string in deep_string_coerce function

Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25394:
URL: https://github.com/apache/airflow/pull/25394#issuecomment-1203947119

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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] uranusjr commented on a diff in pull request #25394: Do not convert boolean values to string in deep_string_coerce function

Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #25394:
URL: https://github.com/apache/airflow/pull/25394#discussion_r936474033


##########
airflow/providers/databricks/utils/databricks.py:
##########
@@ -23,16 +23,25 @@
 from airflow.providers.databricks.hooks.databricks import RunState
 
 
-def deep_string_coerce(content, json_path: str = 'json') -> Union[str, list, dict]:
+def normalise_json_content(content, json_path: str = 'json') -> Union[str, bool, list, dict]:
     """
-    Coerces content or all values of content if it is a dict to a string. The
-    function will throw if content contains non-string or non-numeric types.
+    Normalises content or all values of content if it is a dict to a string. The
+    function will throw if content contains non-string or non-numeric non-boolean types.
     The reason why we have this function is because the ``self.json`` field must be a
     dict with only string values. This is because ``render_template`` will fail
     for numerical values.
+
+    The only one exception is when we have boolean values, they can not be converted
+    to string type because databricks does not understand 'True' or 'False' values.
     """
-    coerce = deep_string_coerce
-    if isinstance(content, str):
+    normalise = normalise_json_content
+    if isinstance(
+        content,
+        (
+            str,
+            bool,
+        ),
+    ):

Review Comment:
   ```suggestion
       if isinstance(content, (str, bool)):
   ```
   
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk commented on pull request #25394: Do not convert boolean values to string in deep_string_coerce function

Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25394:
URL: https://github.com/apache/airflow/pull/25394#issuecomment-1202428470

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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] jgr-trackunit commented on pull request #25394: Do not convert boolean values to string in deep_string_coerce function

Posted by GitBox <gi...@apache.org>.
jgr-trackunit commented on PR #25394:
URL: https://github.com/apache/airflow/pull/25394#issuecomment-1203906845

   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.
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk merged pull request #25394: Do not convert boolean values to string in deep_string_coerce function

Posted by GitBox <gi...@apache.org>.
potiuk merged PR #25394:
URL: https://github.com/apache/airflow/pull/25394


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk commented on pull request #25394: Do not convert boolean values to string in deep_string_coerce function

Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25394:
URL: https://github.com/apache/airflow/pull/25394#issuecomment-1203887955

   > 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. 
   
   BTW. I am the release manager of providers. 
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] jgr-trackunit commented on pull request #25394: Do not convert boolean values to string in deep_string_coerce function

Posted by GitBox <gi...@apache.org>.
jgr-trackunit commented on PR #25394:
URL: https://github.com/apache/airflow/pull/25394#issuecomment-1203846737

   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?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk commented on pull request #25394: Do not convert boolean values to string in deep_string_coerce function

Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25394:
URL: https://github.com/apache/airflow/pull/25394#issuecomment-1203890615

   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 before: https://github.com/apache/airflow/issues?q=is%3Aissue+status+of+testing+is%3Aclosed+label%3A%22testing+status%22


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk commented on pull request #25394: Do not convert boolean values to string in deep_string_coerce function

Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25394:
URL: https://github.com/apache/airflow/pull/25394#issuecomment-1202353906

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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] uranusjr commented on pull request #25394: Do not convert boolean values to string in deep_string_coerce function

Posted by GitBox <gi...@apache.org>.
uranusjr commented on PR #25394:
URL: https://github.com/apache/airflow/pull/25394#issuecomment-1204971756

   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
   ```


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] boring-cyborg[bot] commented on pull request #25394: Do not convert boolean values to string in deep_string_coerce function

Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on PR #25394:
URL: https://github.com/apache/airflow/pull/25394#issuecomment-1199084308

   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]( https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) 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](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
   - Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/BREEZE.rst) 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](https://www.apache.org/foundation/policies/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]( https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#coding-style-and-best-practices).
   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
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] jgr-trackunit commented on pull request #25394: Do not convert boolean values to string in deep_string_coerce function

Posted by GitBox <gi...@apache.org>.
jgr-trackunit commented on PR #25394:
URL: https://github.com/apache/airflow/pull/25394#issuecomment-1204822914

   > 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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] boring-cyborg[bot] commented on pull request #25394: Do not convert boolean values to string in deep_string_coerce function

Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on PR #25394:
URL: https://github.com/apache/airflow/pull/25394#issuecomment-1205435177

   Awesome work, congrats on your first merged pull request!
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org