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/04/30 03:00:37 UTC
[GitHub] [airflow] Isaacwhyuenac opened a new pull request #15609: Update s3.py
Isaacwhyuenac opened a new pull request #15609:
URL: https://github.com/apache/airflow/pull/15609
The changes improves the process of finding the S3 files and gives developer the freedom to list what they want.
For example, suppose in s3 bucket there are
```
2021-04-30 10:38:27 11.7 KiB agg/user_segmentation/tag_article_user_analysis/partition_0=2021-04-29/20210430_023708_00016_g2f4v_31a98366-e1a0-428e-a57f-3c4b0fe2bb79
2021-04-30 10:38:27 49.1 KiB agg/user_segmentation/tag_article_user_analysis/partition_0=2021-04-29/20210430_023708_00016_g2f4v_55438602-3787-4c09-9fad-562e7a6786cb
2021-04-30 10:38:31 10.6 KiB agg/user_segmentation/tag_article_user_analysis/partition_0=2021-04-29/20210430_023708_00016_g2f4v_6773215f-1697-4c99-9e94-f7961e86af62
2021-04-30 10:38:31 27.1 KiB agg/user_segmentation/tag_article_user_analysis/partition_0=2021-04-29/20210430_023708_00016_g2f4v_69c952f5-97b4-45e9-b790-fc7830fb2150
2021-04-30 10:38:31 131.2 KiB agg/user_segmentation/tag_article_user_analysis/partition_0=2021-04-29/20210430_023708_00016_g2f4v_b4b995f5-211d-4d46-bd9a-86912b29d978
2021-04-30 10:38:27 166.2 KiB agg/user_segmentation/tag_article_user_analysis/partition_0=2021-04-29/20210430_023708_00016_g2f4v_bbcebd80-c280-4e66-9431-9a626df8bc33
2021-04-30 10:38:30 171.6 KiB agg/user_segmentation/tag_article_user_analysis/partition_0=2021-04-29/20210430_023708_00016_g2f4v_f4ef423f-cf70-4f71-960e-70f1bdddaf3d
2021-04-30 10:38:27 11.7 KiB agg/user_segmentation/tag_article_user_analysis_v2/partition_0=2021-04-29/20210430_023708_00016_g2f4v_31a98366-e1a0-428e-a57f-3c4b0fe2bb79
2021-04-30 10:38:27 49.1 KiB agg/user_segmentation/tag_article_user_analysis_v2/partition_0=2021-04-29/20210430_023708_00016_g2f4v_55438602-3787-4c09-9fad-562e7a6786cb
2021-04-30 10:38:31 10.6 KiB agg/user_segmentation/tag_article_user_analysis_v2/partition_0=2021-04-29/20210430_023708_00016_g2f4v_6773215f-1697-4c99-9e94-f7961e86af62
2021-04-30 10:38:31 27.1 KiB agg/user_segmentation/tag_article_user_analysis_v2/partition_0=2021-04-29/20210430_023708_00016_g2f4v_69c952f5-97b4-45e9-b790-fc7830fb2150
2021-04-30 10:38:31 131.2 KiB agg/user_segmentation/tag_article_user_analysis_v2/partition_0=2021-04-29/20210430_023708_00016_g2f4v_b4b995f5-211d-4d46-bd9a-86912b29d978
2021-04-30 10:38:27 166.2 KiB agg/user_segmentation/tag_article_user_analysis_v2/partition_0=2021-04-29/20210430_023708_00016_g2f4v_bbcebd80-c280-4e66-9431-9a626df8bc33
2021-04-30 10:38:30 171.6 KiB agg/user_segmentation/tag_article_user_analysis_v2/partition_0=2021-04-29/20210430_023708_00016_g2f4v_f4ef423f-cf70-4f71-960e-70f1bdddaf3d
```
Under the current schema, these two patterns are matched under `agg/user_segmentation/tag_article_user_analysis/` as the trailing slash is stripped of. Developer should have the freedom to choose what pattern they want to match instead of forcing a pattern matching for them.
--
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] eladkal commented on pull request #15609: Remove the `not-allow-trailing-slash` rule on S3_hook
Posted by GitBox <gi...@apache.org>.
eladkal commented on pull request #15609:
URL: https://github.com/apache/airflow/pull/15609#issuecomment-841118423
tests are failing:
```
_______________________ TestAwsS3Hook.test_parse_s3_url ________________________
self = <tests.providers.amazon.aws.hooks.test_s3.TestAwsS3Hook object at 0x7f674ab4dda0>
def test_parse_s3_url(self):
parsed = S3Hook.parse_s3_url("s3://test/this/is/not/a-real-key.txt")
> assert parsed == ("test", "this/is/not/a-real-key.txt"), "Incorrect parsing of the s3 url"
E AssertionError: Incorrect parsing of the s3 url
E assert ('test', '/th...real-key.txt') == ('test', 'thi...real-key.txt')
E At index 1 diff: '/this/is/not/a-real-key.txt' != 'this/is/not/a-real-key.txt'
E Use -v to get the full diff
tests/providers/amazon/aws/hooks/test_s3.py:70: AssertionError
```
--
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] potiuk commented on pull request #15609: Remove the `not-allow-trailing-slash` rule on S3_hook
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #15609:
URL: https://github.com/apache/airflow/pull/15609#issuecomment-849484613
Cool :)
--
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] potiuk commented on pull request #15609: Remove the `not-allow-trailing-slash` rule on S3_hook
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #15609:
URL: https://github.com/apache/airflow/pull/15609#issuecomment-848910243
Running
--
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 #15609: Remove the `not-allow-trailing-slash` rule on S3_hook
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #15609:
URL: https://github.com/apache/airflow/pull/15609#issuecomment-849484516
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] potiuk commented on a change in pull request #15609: Remove the `not-allow-trailing-slash` rule on S3_hook
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #15609:
URL: https://github.com/apache/airflow/pull/15609#discussion_r634123965
##########
File path: UPDATING.md
##########
@@ -100,6 +100,10 @@ serve as a DagBag cache burst time.
The `default_queue` configuration option has been moved from `[celery]` section to `[operators]` section to allow for re-use between different executors.
+### Update `parse_s3_url` in airflow.providers.amazon.aws.hooks.s3.S3Hook
+
+The `parse_s3_url` method in `airflow.providers.amazon.aws.hooks.s3.S3Hook` will now not remove the trailing slash by default. This will allow user to more flexibly implement their preferred prefix-level or subdirectory-level s3 operations.
+
Review comment:
Correct
--
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] feluelle commented on a change in pull request #15609: Remove the `not-allow-trailing-slash` rule on S3_hook
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #15609:
URL: https://github.com/apache/airflow/pull/15609#discussion_r626270614
##########
File path: airflow/providers/amazon/aws/hooks/s3.py
##########
@@ -143,9 +143,9 @@ def parse_s3_url(s3url: str) -> Tuple[str, str]:
raise AirflowException(f'Please provide a bucket_name instead of "{s3url}"')
bucket_name = parsed_url.netloc
- key = parsed_url.path.strip('/')
+ # key = parsed_url.path.strip('/')
Review comment:
```suggestion
key = parsed_url.path
```
##########
File path: airflow/providers/amazon/aws/hooks/s3.py
##########
@@ -143,9 +143,9 @@ def parse_s3_url(s3url: str) -> Tuple[str, str]:
raise AirflowException(f'Please provide a bucket_name instead of "{s3url}"')
bucket_name = parsed_url.netloc
- key = parsed_url.path.strip('/')
+ # key = parsed_url.path.strip('/')
- return bucket_name, key
+ return bucket_name, parsed_url.path
Review comment:
```suggestion
return bucket_name, key
```
--
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] Isaacwhyuenac commented on pull request #15609: Remove the `not-allow-trailing-slash` rule on S3_hook
Posted by GitBox <gi...@apache.org>.
Isaacwhyuenac commented on pull request #15609:
URL: https://github.com/apache/airflow/pull/15609#issuecomment-833631880
Added the unit test. Hope it is useful.
--
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] eladkal commented on a change in pull request #15609: Remove the `not-allow-trailing-slash` rule on S3_hook
Posted by GitBox <gi...@apache.org>.
eladkal commented on a change in pull request #15609:
URL: https://github.com/apache/airflow/pull/15609#discussion_r634581635
##########
File path: UPDATING.md
##########
@@ -100,6 +100,10 @@ serve as a DagBag cache burst time.
The `default_queue` configuration option has been moved from `[celery]` section to `[operators]` section to allow for re-use between different executors.
+### Update `parse_s3_url` in airflow.providers.amazon.aws.hooks.s3.S3Hook
+
+The `parse_s3_url` method in `airflow.providers.amazon.aws.hooks.s3.S3Hook` will now not remove the trailing slash by default. This will allow user to more flexibly implement their preferred prefix-level or subdirectory-level s3 operations.
+
Review comment:
Please add also 2.0.0 in the versions section of the provider yaml
--
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] Isaacwhyuenac edited a comment on pull request #15609: Remove the `not-allow-trailing-slash` rule on S3_hook
Posted by GitBox <gi...@apache.org>.
Isaacwhyuenac edited a comment on pull request #15609:
URL: https://github.com/apache/airflow/pull/15609#issuecomment-848909118
> Could you please rebase on latest master @Isaacwhyuenac ? The tests are failing because it is needed.
@ potiuk
Merged the current master.
--
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 #15609: Update s3.py
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #15609:
URL: https://github.com/apache/airflow/pull/15609#issuecomment-829761790
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] github-actions[bot] commented on pull request #15609: Remove the `not-allow-trailing-slash` rule on S3_hook
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #15609:
URL: https://github.com/apache/airflow/pull/15609#issuecomment-841097487
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest master 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Isaacwhyuenac commented on a change in pull request #15609: Remove the `not-allow-trailing-slash` rule on S3_hook
Posted by GitBox <gi...@apache.org>.
Isaacwhyuenac commented on a change in pull request #15609:
URL: https://github.com/apache/airflow/pull/15609#discussion_r634468895
##########
File path: UPDATING.md
##########
@@ -100,6 +100,10 @@ serve as a DagBag cache burst time.
The `default_queue` configuration option has been moved from `[celery]` section to `[operators]` section to allow for re-use between different executors.
+### Update `parse_s3_url` in airflow.providers.amazon.aws.hooks.s3.S3Hook
+
+The `parse_s3_url` method in `airflow.providers.amazon.aws.hooks.s3.S3Hook` will now not remove the trailing slash by default. This will allow user to more flexibly implement their preferred prefix-level or subdirectory-level s3 operations.
+
Review comment:
@eladkal @potiuk Hope I have resolved your 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] Isaacwhyuenac commented on a change in pull request #15609: Remove the `not-allow-trailing-slash` rule on S3_hook
Posted by GitBox <gi...@apache.org>.
Isaacwhyuenac commented on a change in pull request #15609:
URL: https://github.com/apache/airflow/pull/15609#discussion_r635061356
##########
File path: UPDATING.md
##########
@@ -100,6 +100,10 @@ serve as a DagBag cache burst time.
The `default_queue` configuration option has been moved from `[celery]` section to `[operators]` section to allow for re-use between different executors.
+### Update `parse_s3_url` in airflow.providers.amazon.aws.hooks.s3.S3Hook
+
+The `parse_s3_url` method in `airflow.providers.amazon.aws.hooks.s3.S3Hook` will now not remove the trailing slash by default. This will allow user to more flexibly implement their preferred prefix-level or subdirectory-level s3 operations.
+
Review comment:
@eladkal updated
--
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] potiuk commented on pull request #15609: Remove the `not-allow-trailing-slash` rule on S3_hook
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #15609:
URL: https://github.com/apache/airflow/pull/15609#issuecomment-848803922
Could you please rebase on latest master @Isaacwhyuenac ? The tests are failing because it is needed.
--
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] potiuk merged pull request #15609: Remove the `not-allow-trailing-slash` rule on S3_hook
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #15609:
URL: https://github.com/apache/airflow/pull/15609
--
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] eladkal commented on a change in pull request #15609: Remove the `not-allow-trailing-slash` rule on S3_hook
Posted by GitBox <gi...@apache.org>.
eladkal commented on a change in pull request #15609:
URL: https://github.com/apache/airflow/pull/15609#discussion_r634107986
##########
File path: UPDATING.md
##########
@@ -100,6 +100,10 @@ serve as a DagBag cache burst time.
The `default_queue` configuration option has been moved from `[celery]` section to `[operators]` section to allow for re-use between different executors.
+### Update `parse_s3_url` in airflow.providers.amazon.aws.hooks.s3.S3Hook
+
+The `parse_s3_url` method in `airflow.providers.amazon.aws.hooks.s3.S3Hook` will now not remove the trailing slash by default. This will allow user to more flexibly implement their preferred prefix-level or subdirectory-level s3 operations.
+
Review comment:
The change of this PR effects only Amazon provider so the note should be in the provider change log https://github.com/apache/airflow/blob/master/airflow/providers/amazon/CHANGELOG.rst
If this change is not backward compatible probably we need to bump major version in the [provider yaml](https://github.com/apache/airflow/blob/master/airflow/providers/amazon/provider.yaml#L24) @potiuk this means 2.0.0 correct?
--
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] Isaacwhyuenac commented on pull request #15609: Remove the `not-allow-trailing-slash` rule on S3_hook
Posted by GitBox <gi...@apache.org>.
Isaacwhyuenac commented on pull request #15609:
URL: https://github.com/apache/airflow/pull/15609#issuecomment-849297728
> Running
All Passed! @potiuk @eladkal
--
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] feluelle commented on a change in pull request #15609: Remove the `not-allow-trailing-slash` rule on S3_hook
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #15609:
URL: https://github.com/apache/airflow/pull/15609#discussion_r632349591
##########
File path: airflow/providers/amazon/aws/hooks/s3.py
##########
@@ -143,9 +143,9 @@ def parse_s3_url(s3url: str) -> Tuple[str, str]:
raise AirflowException(f'Please provide a bucket_name instead of "{s3url}"')
bucket_name = parsed_url.netloc
- key = parsed_url.path.strip('/')
+ # key = parsed_url.path.strip('/')
- return bucket_name, key
+ return bucket_name, parsed_url.path
Review comment:
Please use the `key`. It is much more readable instead of `parsed_url.path`
--
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] Isaacwhyuenac edited a comment on pull request #15609: Remove the `not-allow-trailing-slash` rule on S3_hook
Posted by GitBox <gi...@apache.org>.
Isaacwhyuenac edited a comment on pull request #15609:
URL: https://github.com/apache/airflow/pull/15609#issuecomment-849297728
> Running
All tests Passed! @potiuk @eladkal
--
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] Isaacwhyuenac commented on pull request #15609: Remove the `not-allow-trailing-slash` rule on S3_hook
Posted by GitBox <gi...@apache.org>.
Isaacwhyuenac commented on pull request #15609:
URL: https://github.com/apache/airflow/pull/15609#issuecomment-841144672
Fixed @eladkal @feluelle
--
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] Isaacwhyuenac commented on pull request #15609: Remove the `not-allow-trailing-slash` rule on S3_hook
Posted by GitBox <gi...@apache.org>.
Isaacwhyuenac commented on pull request #15609:
URL: https://github.com/apache/airflow/pull/15609#issuecomment-848909118
> Could you please rebase on latest master @Isaacwhyuenac ? The tests are failing because it is needed.
@ potiuk
Merged the current master.
--
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] Isaacwhyuenac commented on a change in pull request #15609: Remove the `not-allow-trailing-slash` rule on S3_hook
Posted by GitBox <gi...@apache.org>.
Isaacwhyuenac commented on a change in pull request #15609:
URL: https://github.com/apache/airflow/pull/15609#discussion_r639656742
##########
File path: airflow/providers/amazon/provider.yaml
##########
@@ -22,6 +22,7 @@ description: |
Amazon integration (including `Amazon Web Services (AWS) <https://aws.amazon.com/>`__).
versions:
+ - 2.0.0
Review comment:
@potiuk updated
--
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] Isaacwhyuenac commented on a change in pull request #15609: Remove the `not-allow-trailing-slash` rule on S3_hook
Posted by GitBox <gi...@apache.org>.
Isaacwhyuenac commented on a change in pull request #15609:
URL: https://github.com/apache/airflow/pull/15609#discussion_r639656388
##########
File path: airflow/providers/amazon/CHANGELOG.rst
##########
@@ -19,6 +19,15 @@
Changelog
---------
+2.0.0
Review comment:
@potiuk updated
--
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] Isaacwhyuenac edited a comment on pull request #15609: Remove the `not-allow-trailing-slash` rule on S3_hook
Posted by GitBox <gi...@apache.org>.
Isaacwhyuenac edited a comment on pull request #15609:
URL: https://github.com/apache/airflow/pull/15609#issuecomment-841144672
Fixed @eladkal @feluelle . Hope it will solve potential issue
--
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] potiuk commented on a change in pull request #15609: Remove the `not-allow-trailing-slash` rule on S3_hook
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #15609:
URL: https://github.com/apache/airflow/pull/15609#discussion_r639633677
##########
File path: airflow/providers/amazon/CHANGELOG.rst
##########
@@ -19,6 +19,15 @@
Changelog
---------
+2.0.0
Review comment:
Could you please remove the changelog entry :) . I am fully automating changelog preparation for the next release and from then on there will be no need to add any links here by committers, except if there will be a breaking change requiring explanation.
##########
File path: airflow/providers/amazon/provider.yaml
##########
@@ -22,6 +22,7 @@ description: |
Amazon integration (including `Amazon Web Services (AWS) <https://aws.amazon.com/>`__).
versions:
+ - 2.0.0
Review comment:
Same here.
--
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] feluelle commented on a change in pull request #15609: Remove the `not-allow-trailing-slash` rule on S3_hook
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #15609:
URL: https://github.com/apache/airflow/pull/15609#discussion_r632351089
##########
File path: tests/providers/amazon/aws/hooks/test_s3.py
##########
@@ -69,6 +69,11 @@ def test_parse_s3_url(self):
parsed = S3Hook.parse_s3_url("s3://test/this/is/not/a-real-key.txt")
assert parsed == ("test", "this/is/not/a-real-key.txt"), "Incorrect parsing of the s3 url"
+ def test_parse_s3_object_directory(self):
+ parsed = S3Hook.parse_s3_url("s3://test/this/is/not/a-real-s3-directory/")
+ assert parsed != ("test", "this/is/not/a-real-s3-directory"), "Incorrect parsing of the s3 url"
+ assert parsed == ("test", "this/is/not/a-real-s3-directory/"), "Incorrect parsing of the s3 url"
Review comment:
```suggestion
parsed = S3Hook.parse_s3_url("s3://test/this/is/not/a-real-s3-directory/")
assert parsed == ("test", "this/is/not/a-real-s3-directory/"), "Incorrect parsing of the s3 url"
```
This is sufficient :) If the `==` fails, the other one will succeed automatically.
--
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] Isaacwhyuenac commented on pull request #15609: Remove the `not-allow-trailing-slash` rule on S3_hook
Posted by GitBox <gi...@apache.org>.
Isaacwhyuenac commented on pull request #15609:
URL: https://github.com/apache/airflow/pull/15609#issuecomment-841084430
I have updated according to your suggestions. @feluelle. Hope you can take a look.
--
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