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/02/20 08:14:55 UTC
[GitHub] [airflow] kadai0308 opened a new pull request #21694: fix maxsplit of line split (by '=') in _parse_env_file function
kadai0308 opened a new pull request #21694:
URL: https://github.com/apache/airflow/pull/21694
related: #21625
If there is another '=' in the conn string in the env file, it will been splited into 3 sub-string which is not expected( We expect it only been splited into 2 sub-string, key and value ).
It can been fixed by correct the maxsplit to 1 of line split in _parse_env_file function in LocalFileSystem secrets.
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
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] mik-laj commented on pull request #21694: set maxsplit to be 1 of line split in _parse_env_file function
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #21694:
URL: https://github.com/apache/airflow/pull/21694#issuecomment-1046308054
Can you add tests to avoid regression?
--
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] github-actions[bot] commented on pull request #21694: set maxsplit to be 1 of line split in _parse_env_file function
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #21694:
URL: https://github.com/apache/airflow/pull/21694#issuecomment-1046635447
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
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 change in pull request #21694: set maxsplit to be 1 of line split in _parse_env_file function
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #21694:
URL: https://github.com/apache/airflow/pull/21694#discussion_r810834007
##########
File path: airflow/secrets/local_filesystem.py
##########
@@ -74,7 +74,7 @@ def _parse_env_file(file_path: str) -> Tuple[Dict[str, List[str]], List[FileSynt
# Ignore comments
continue
- var_parts: List[str] = line.split("=", 2)
+ var_parts: List[str] = line.split("=", 1)
Review comment:
It would be easier to use `partition` here.
```python
key, sep, value = line.partition("=")
if not sep:
errors.append(...)
continue
if not key:
errors.append(...)
```
--
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] kadai0308 commented on a change in pull request #21694: set maxsplit to be 1 of line split in _parse_env_file function
Posted by GitBox <gi...@apache.org>.
kadai0308 commented on a change in pull request #21694:
URL: https://github.com/apache/airflow/pull/21694#discussion_r810900838
##########
File path: airflow/secrets/local_filesystem.py
##########
@@ -74,7 +74,7 @@ def _parse_env_file(file_path: str) -> Tuple[Dict[str, List[str]], List[FileSynt
# Ignore comments
continue
- var_parts: List[str] = line.split("=", 2)
+ var_parts: List[str] = line.split("=", 1)
Review comment:
Thanks for your comment, it did looks more elegant, let me update the code by using partition.
Also nice to meet you. :)
--
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] kadai0308 commented on a change in pull request #21694: set maxsplit to be 1 of line split in _parse_env_file function
Posted by GitBox <gi...@apache.org>.
kadai0308 commented on a change in pull request #21694:
URL: https://github.com/apache/airflow/pull/21694#discussion_r810900838
##########
File path: airflow/secrets/local_filesystem.py
##########
@@ -74,7 +74,7 @@ def _parse_env_file(file_path: str) -> Tuple[Dict[str, List[str]], List[FileSynt
# Ignore comments
continue
- var_parts: List[str] = line.split("=", 2)
+ var_parts: List[str] = line.split("=", 1)
Review comment:
Thanks for your comment, it did looks more elegant, let me update the code by using partition.
Also nice to meet you.
--
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] kadai0308 commented on pull request #21694: set maxsplit to be 1 of line split in _parse_env_file function
Posted by GitBox <gi...@apache.org>.
kadai0308 commented on pull request #21694:
URL: https://github.com/apache/airflow/pull/21694#issuecomment-1046409532
> Can you add tests to avoid regression?
Sure
--
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 merged pull request #21694: Correctly handle multiple '=' in LocalFileSystem secrets.
Posted by GitBox <gi...@apache.org>.
uranusjr merged pull request #21694:
URL: https://github.com/apache/airflow/pull/21694
--
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 #21694: Correctly handle multiple '=' in LocalFileSystem secrets.
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #21694:
URL: https://github.com/apache/airflow/pull/21694#issuecomment-1049576639
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
[GitHub] [airflow] kadai0308 commented on a change in pull request #21694: set maxsplit to be 1 of line split in _parse_env_file function
Posted by GitBox <gi...@apache.org>.
kadai0308 commented on a change in pull request #21694:
URL: https://github.com/apache/airflow/pull/21694#discussion_r810900838
##########
File path: airflow/secrets/local_filesystem.py
##########
@@ -74,7 +74,7 @@ def _parse_env_file(file_path: str) -> Tuple[Dict[str, List[str]], List[FileSynt
# Ignore comments
continue
- var_parts: List[str] = line.split("=", 2)
+ var_parts: List[str] = line.split("=", 1)
Review comment:
Thanks for your comment, it did looks more elegant, let me update the code by using partition.
--
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 #21694: fix maxsplit of line split (by '=') in _parse_env_file function
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #21694:
URL: https://github.com/apache/airflow/pull/21694#issuecomment-1046185659
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