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 2021/01/28 22:40:58 UTC

[GitHub] [airflow] levahim opened a new pull request #13962: Fixed reading from zip package to default to text.

levahim opened a new pull request #13962:
URL: https://github.com/apache/airflow/pull/13962


   The `open_maybe_zipped` function returns different file-like objects depending on whether it's called for a plain file or for a file in a zip archive. The problem is that by default `io.open` (used for plain files) returns file in text mode and subsequent `read` on it returns strings. `ZipFile` on the other hand by default returns a binary file and subsequent `read` on it returns bytes.
   The returned value for `open_maybe_zipped` should be the same regardless whether it's a zip or a plain file--it should be in text mode. Returning binaries for zip packages causes problems. For example, when saving DAG code is turned on, the `DagCode` model tries to save DAG source code in the metadata database. SQLAlchemy throws an error for DAGs that come from a zip package, because tries to save binary value in a string column.
   This PR fixes the problem.


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

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

[GitHub] [airflow] mik-laj merged pull request #13962: Fixed reading from zip package to default to text.

Posted by GitBox <gi...@apache.org>.

mik-laj merged pull request #13962:
URL: https://github.com/apache/airflow/pull/13962


   


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

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

[GitHub] [airflow] boring-cyborg[bot] commented on pull request #13962: Fixed reading from zip package to default to text.

Posted by GitBox <gi...@apache.org>.

boring-cyborg[bot] commented on pull request #13962:
URL: https://github.com/apache/airflow/pull/13962#issuecomment-769446888

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/master/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, pylint and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/master/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/master/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/master/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/master/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.

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

[GitHub] [airflow] levahim commented on pull request #13962: Fixed reading from zip package to default to text.

Posted by GitBox <gi...@apache.org>.

levahim commented on pull request #13962:
URL: https://github.com/apache/airflow/pull/13962#issuecomment-770205515


   OK, will do. Not sure how a one-liner fix like the one in this PR can break the tests though, unless some tests specifically rely on the `airflow.utils.open_maybe_zipped` function return binary data in some cases (which would be testing for the incorrect function behavior). Let me check...


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

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

[GitHub] [airflow] levahim commented on pull request #13962: Fixed reading from zip package to default to text.

Posted by GitBox <gi...@apache.org>.

levahim commented on pull request #13962:
URL: https://github.com/apache/airflow/pull/13962#issuecomment-770247910


   Found the problem, fixed the unit test. Resubmitted as PR #13984


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

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

[GitHub] [airflow] boring-cyborg[bot] commented on pull request #13962: Fixed reading from zip package to default to text.

Posted by GitBox <gi...@apache.org>.

boring-cyborg[bot] commented on pull request #13962:
URL: https://github.com/apache/airflow/pull/13962#issuecomment-769639991


   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.

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

[GitHub] [airflow] kaxil commented on pull request #13962: Fixed reading from zip package to default to text.

Posted by GitBox <gi...@apache.org>.

kaxil commented on pull request #13962:
URL: https://github.com/apache/airflow/pull/13962#issuecomment-769980017


   The tests were failing for this PR -- so we had to revert as it broke the master, can you create a new PR please and investigate why the unit tests were failing @levahim 


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

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