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/12/19 10:46:42 UTC
[GitHub] [airflow] vsimon opened a new pull request #19700: Fix parsing of Cloudwatch log group arn containing slashes (#14667)
vsimon opened a new pull request #19700:
URL: https://github.com/apache/airflow/pull/19700
When the log group arn contains slashes, the urlparse function parses
the group name as part of the 'path' instead of including it as part of
the 'netloc'. This fix concatenates both the 'netloc' and 'path' fields
together to use as the group arn. For group names without slashes, the
functionality remains the same as the group name is still parsed as part
of the 'netloc' field and the 'path' field will be empty.
Fix #19783.
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ 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] uranusjr commented on pull request #19700: Fix parsing of Cloudwatch log group arn containing slashes (#14667)
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #19700:
URL: https://github.com/apache/airflow/pull/19700#issuecomment-974338923
Could you add a test case showing what’s failing previously and fixed by this?
--
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] vsimon commented on pull request #19700: Fix parsing of Cloudwatch log group arn containing slashes (#14667)
Posted by GitBox <gi...@apache.org>.
vsimon commented on pull request #19700:
URL: https://github.com/apache/airflow/pull/19700#issuecomment-997312711
gentle ping @uranusjr, would you have the power to merge PRs?
--
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] vsimon commented on pull request #19700: Fix parsing of Cloudwatch log group arn containing slashes (#14667)
Posted by GitBox <gi...@apache.org>.
vsimon commented on pull request #19700:
URL: https://github.com/apache/airflow/pull/19700#issuecomment-985328186
Thanks @o-nikolas @uranusjr. I think I was able to add a concise unit test which fails in main branch and passes in this branch. Let me know if the approach is off. As for where to put it, I thought `tests/core/test_logging_config.py` might be a relevant location.
--
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 closed pull request #19700: Fix parsing of Cloudwatch log group arn containing slashes (#14667)
Posted by GitBox <gi...@apache.org>.
potiuk closed pull request #19700:
URL: https://github.com/apache/airflow/pull/19700
--
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 #19700: Fix parsing of Cloudwatch log group arn containing slashes (#14667)
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #19700:
URL: https://github.com/apache/airflow/pull/19700#issuecomment-984308076
A gentle ping.
--
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] vsimon commented on pull request #19700: Fix parsing of Cloudwatch log group arn containing slashes (#14667)
Posted by GitBox <gi...@apache.org>.
vsimon commented on pull request #19700:
URL: https://github.com/apache/airflow/pull/19700#issuecomment-974371970
Thank you @uranusjr for finding this, looks definitely applicable.
First thought is to do something similar with a new file/tests at `tests/providers/amazon/aws/log/test_cloudwatch_task_system_handler.py`
--
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] vsimon commented on a change in pull request #19700: Fix parsing of Cloudwatch log group arn containing slashes (#14667)
Posted by GitBox <gi...@apache.org>.
vsimon commented on a change in pull request #19700:
URL: https://github.com/apache/airflow/pull/19700#discussion_r752937379
##########
File path: airflow/config_templates/airflow_local_settings.py
##########
@@ -194,12 +194,13 @@
DEFAULT_LOGGING_CONFIG['handlers'].update(S3_REMOTE_HANDLERS)
elif REMOTE_BASE_LOG_FOLDER.startswith('cloudwatch://'):
+ url_parts = urlparse(REMOTE_BASE_LOG_FOLDER)
CLOUDWATCH_REMOTE_HANDLERS: Dict[str, Dict[str, str]] = {
'task': {
'class': 'airflow.providers.amazon.aws.log.cloudwatch_task_handler.CloudwatchTaskHandler',
'formatter': 'airflow',
'base_log_folder': str(os.path.expanduser(BASE_LOG_FOLDER)),
- 'log_group_arn': urlparse(REMOTE_BASE_LOG_FOLDER).netloc,
+ 'log_group_arn': ''.join(url_parts.netloc + url_parts.path),
Review comment:
totally right
--
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] o-nikolas commented on pull request #19700: Fix parsing of Cloudwatch log group arn containing slashes (#14667)
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on pull request #19700:
URL: https://github.com/apache/airflow/pull/19700#issuecomment-990224199
> A gentle ping. @o-nikolas @uranusjr
Yupp, lgtm, I approved the latest changes last week. But I don't have the power to merge PRs unfortunately.
--
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] vsimon edited a comment on pull request #19700: Fix parsing of Cloudwatch log group arn containing slashes (#14667)
Posted by GitBox <gi...@apache.org>.
vsimon edited a comment on pull request #19700:
URL: https://github.com/apache/airflow/pull/19700#issuecomment-974341547
Hi @uranusjr I tried to find an existing test case bed that I could slot this in nicely, but I couldn't find one. Do you know if some tests related to this functionality already exists (for testing config templates or local settings) or some guidance on which folder in tests/ would be most appropriate to build out a new test?
--
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 #19700: Fix parsing of Cloudwatch log group arn containing slashes (#14667)
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #19700:
URL: https://github.com/apache/airflow/pull/19700#issuecomment-974362260
There are a few of tests on this particular part of setup:
* `tests/providers/google/cloud/log/test_stackdriver_task_handler_system.py` has tests on `REMOTE_BASE_LOG_FOLDER` with the `stackdriver://` prefix.
* `tests/providers/google/cloud/log/test_gcs_task_handler_system.py` has one test for `gs://`.
They might help with coming up a test for `cloudwatch://`.
--
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] o-nikolas commented on pull request #19700: Fix parsing of Cloudwatch log group arn containing slashes (#14667)
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on pull request #19700:
URL: https://github.com/apache/airflow/pull/19700#issuecomment-984945769
> For this two-line change, is there a way for it to be part of an existing unit-test or with the nature of this it must be a system-level test?
IMO this particular change does not need system level testing, and is perfect for a small unit test. As for where to put that unit test, that's a more difficult question.
--
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] vsimon commented on pull request #19700: Fix parsing of Cloudwatch log group arn containing slashes (#14667)
Posted by GitBox <gi...@apache.org>.
vsimon commented on pull request #19700:
URL: https://github.com/apache/airflow/pull/19700#issuecomment-990192339
A gentle ping. @o-nikolas @uranusjr
--
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 #19700: Fix parsing of Cloudwatch log group arn containing slashes (#14667)
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #19700:
URL: https://github.com/apache/airflow/pull/19700#discussion_r752933036
##########
File path: airflow/config_templates/airflow_local_settings.py
##########
@@ -194,12 +194,13 @@
DEFAULT_LOGGING_CONFIG['handlers'].update(S3_REMOTE_HANDLERS)
elif REMOTE_BASE_LOG_FOLDER.startswith('cloudwatch://'):
+ url_parts = urlparse(REMOTE_BASE_LOG_FOLDER)
CLOUDWATCH_REMOTE_HANDLERS: Dict[str, Dict[str, str]] = {
'task': {
'class': 'airflow.providers.amazon.aws.log.cloudwatch_task_handler.CloudwatchTaskHandler',
'formatter': 'airflow',
'base_log_folder': str(os.path.expanduser(BASE_LOG_FOLDER)),
- 'log_group_arn': urlparse(REMOTE_BASE_LOG_FOLDER).netloc,
+ 'log_group_arn': ''.join(url_parts.netloc + url_parts.path),
Review comment:
```suggestion
'log_group_arn': url_parts.netloc + url_parts.path,
```
These are strings, `"".join()` is not meaningful.
--
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 #19700: Fix parsing of Cloudwatch log group arn containing slashes (#14667)
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #19700:
URL: https://github.com/apache/airflow/pull/19700
--
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] vsimon commented on pull request #19700: Fix parsing of Cloudwatch log group arn containing slashes (#14667)
Posted by GitBox <gi...@apache.org>.
vsimon commented on pull request #19700:
URL: https://github.com/apache/airflow/pull/19700#issuecomment-984832267
hi @uranusjr I haven't forgotten about this, adding a new system test there's a bit of a learning curve, inheriting from AmazonSystemTest, SystemTest and understanding what those do and what new functionality that may need to be added.
For this two-line change, is there a way for it to be part of an existing unit-test or with the nature of this it must be a system-level test?
--
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 #19700: Fix parsing of Cloudwatch log group arn containing slashes (#14667)
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #19700:
URL: https://github.com/apache/airflow/pull/19700#issuecomment-985410807
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] vsimon commented on pull request #19700: Fix parsing of Cloudwatch log group arn containing slashes (#14667)
Posted by GitBox <gi...@apache.org>.
vsimon commented on pull request #19700:
URL: https://github.com/apache/airflow/pull/19700#issuecomment-974341547
Hi @uranusjr I tried to find an existing test case bed that I could slot this in nicely, but I couldn't find one. Do you know if some tests related to this functionality already exists?
--
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] vsimon commented on a change in pull request #19700: Fix parsing of Cloudwatch log group arn containing slashes (#14667)
Posted by GitBox <gi...@apache.org>.
vsimon commented on a change in pull request #19700:
URL: https://github.com/apache/airflow/pull/19700#discussion_r752937379
##########
File path: airflow/config_templates/airflow_local_settings.py
##########
@@ -194,12 +194,13 @@
DEFAULT_LOGGING_CONFIG['handlers'].update(S3_REMOTE_HANDLERS)
elif REMOTE_BASE_LOG_FOLDER.startswith('cloudwatch://'):
+ url_parts = urlparse(REMOTE_BASE_LOG_FOLDER)
CLOUDWATCH_REMOTE_HANDLERS: Dict[str, Dict[str, str]] = {
'task': {
'class': 'airflow.providers.amazon.aws.log.cloudwatch_task_handler.CloudwatchTaskHandler',
'formatter': 'airflow',
'base_log_folder': str(os.path.expanduser(BASE_LOG_FOLDER)),
- 'log_group_arn': urlparse(REMOTE_BASE_LOG_FOLDER).netloc,
+ 'log_group_arn': ''.join(url_parts.netloc + url_parts.path),
Review comment:
totally right thanks
--
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] vsimon edited a comment on pull request #19700: Fix parsing of Cloudwatch log group arn containing slashes (#14667)
Posted by GitBox <gi...@apache.org>.
vsimon edited a comment on pull request #19700:
URL: https://github.com/apache/airflow/pull/19700#issuecomment-974341547
--
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 #19700: Fix parsing of Cloudwatch log group arn containing slashes (#14667)
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #19700:
URL: https://github.com/apache/airflow/pull/19700#issuecomment-997369586
There's something wrong with Github Actions. Let me close/reopen to rebuild.
--
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