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 2019/12/29 23:10:15 UTC
[GitHub] [airflow] nuclearpinguin opened a new pull request #6957:
[AIRFLOW-6396] Use tempfile.TemporaryDirectory instead of custom one
nuclearpinguin opened a new pull request #6957: [AIRFLOW-6396] Use tempfile.TemporaryDirectory instead of custom one
URL: https://github.com/apache/airflow/pull/6957
Make sure you have checked _all_ steps below.
### Jira
- [ ] My PR addresses the following [Airflow Jira](https://issues.apache.org/jira/browse/AIRFLOW/) issues and references them in the PR title. For example, "\[AIRFLOW-XXX\] My Airflow PR"
- https://issues.apache.org/jira/browse/AIRFLOW-6396
### Description
- [ ] Here are some details about my PR, including screenshots of any UI changes:
Removing `airflow.utils.file.TemporaryDirectory` because the same functionality is provided by `tempfile.TemporaryDirectory`.
### Tests
- [ ] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
### Commits
- [ ] My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Documentation
- [ ] In case of new functionality, my PR adds documentation that describes how to use it.
- All the public functions and the classes in the PR contain docstrings that explain what it does
- If you implement backwards incompatible changes, please leave a note in the [Updating.md](https://github.com/apache/airflow/blob/master/UPDATING.md) so we can assign it to a appropriate release
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on a change in pull request #6957:
[AIRFLOW-6396] Use tempfile.TemporaryDirectory instead of custom one
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #6957: [AIRFLOW-6396] Use tempfile.TemporaryDirectory instead of custom one
URL: https://github.com/apache/airflow/pull/6957#discussion_r361880066
##########
File path: UPDATING.md
##########
@@ -41,6 +41,10 @@ assists users migrating to a new version.
## Airflow Master
+### Remove airflow.utils.file.TemporaryDirectory
+
+Removing this because the same functionality is provided by `tempfile.TemporaryDirectory`.
Review comment:
If you are interested in the subject, I invite you to read the document below, which contains some good examples.
https://developers.google.com/style/inclusive-documentation
> Not recommended: Before launch, give everything a final sanity-check.
> Recommended: Before launch, give everything a final check for completeness and clarity.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on a change in pull request #6957:
[AIRFLOW-6396] Use tempfile.TemporaryDirectory instead of custom one
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #6957: [AIRFLOW-6396] Use tempfile.TemporaryDirectory instead of custom one
URL: https://github.com/apache/airflow/pull/6957#discussion_r361880104
##########
File path: UPDATING.md
##########
@@ -41,6 +41,10 @@ assists users migrating to a new version.
## Airflow Master
+### Remove airflow.utils.file.TemporaryDirectory
+
+Removing this because the same functionality is provided by `tempfile.TemporaryDirectory`.
Review comment:
I think it's worth doing. I'll do it in a moment.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on a change in pull request #6957:
[AIRFLOW-6396] Use tempfile.TemporaryDirectory instead of custom one
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #6957: [AIRFLOW-6396] Use tempfile.TemporaryDirectory instead of custom one
URL: https://github.com/apache/airflow/pull/6957#discussion_r361882019
##########
File path: UPDATING.md
##########
@@ -41,6 +41,10 @@ assists users migrating to a new version.
## Airflow Master
+### Deprecated airflow.utils.file.TemporaryDirectory
+
+Instead user should use `tempfile.TemporaryDirectory` which provides the same functionality.
Review comment:
Can you add example?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on a change in pull request #6957:
[AIRFLOW-6396] Use tempfile.TemporaryDirectory instead of custom one
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #6957: [AIRFLOW-6396] Use tempfile.TemporaryDirectory instead of custom one
URL: https://github.com/apache/airflow/pull/6957#discussion_r361879362
##########
File path: UPDATING.md
##########
@@ -41,6 +41,10 @@ assists users migrating to a new version.
## Airflow Master
+### Remove airflow.utils.file.TemporaryDirectory
+
+Removing this because the same functionality is provided by `tempfile.TemporaryDirectory`.
Review comment:
Can you add more information here?
In my opinion, a good note should contain the following information:
[ ] Previous behaviors
[ ] New behaviors
[ ] If possible, a simple example of how to make a change
[ ] If possible, the benefit for the user after migration e.g. "we want to make these changes to unify class names."
[ ] If possible, the reason for the change e.g. reference for AIP-21
Here is a similar example that includes most if not all of the requirements:
https://github.com/apache/airflow/blob/master/UPDATING.md#removal-of-redirect_stdout-redirect_stderr
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] codecov-io edited a comment on issue #6957:
[AIRFLOW-6396] Use tempfile.TemporaryDirectory instead of custom one
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #6957: [AIRFLOW-6396] Use tempfile.TemporaryDirectory instead of custom one
URL: https://github.com/apache/airflow/pull/6957#issuecomment-569559227
# [Codecov](https://codecov.io/gh/apache/airflow/pull/6957?src=pr&el=h1) Report
> Merging [#6957](https://codecov.io/gh/apache/airflow/pull/6957?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/5e100ab753635521d33f287bdffd3d9bd4c6ae0f?src=pr&el=desc) will **decrease** coverage by `0.3%`.
> The diff coverage is `69.23%`.
[](https://codecov.io/gh/apache/airflow/pull/6957?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6957 +/- ##
==========================================
- Coverage 84.7% 84.39% -0.31%
==========================================
Files 679 679
Lines 38488 38474 -14
==========================================
- Hits 32600 32472 -128
- Misses 5888 6002 +114
```
| [Impacted Files](https://codecov.io/gh/apache/airflow/pull/6957?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [airflow/hooks/pig\_hook.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9ob29rcy9waWdfaG9vay5weQ==) | `95.34% <100%> (-0.11%)` | :arrow_down: |
| [airflow/operators/bash\_operator.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9vcGVyYXRvcnMvYmFzaF9vcGVyYXRvci5weQ==) | `90.38% <100%> (-0.19%)` | :arrow_down: |
| [airflow/operators/s3\_to\_hive\_operator.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9vcGVyYXRvcnMvczNfdG9faGl2ZV9vcGVyYXRvci5weQ==) | `93.96% <100%> (-0.06%)` | :arrow_down: |
| [airflow/hooks/hive\_hooks.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9ob29rcy9oaXZlX2hvb2tzLnB5) | `77.55% <100%> (-0.06%)` | :arrow_down: |
| [airflow/operators/python\_operator.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9vcGVyYXRvcnMvcHl0aG9uX29wZXJhdG9yLnB5) | `95.8% <100%> (ø)` | :arrow_up: |
| [airflow/contrib/sensors/bash\_sensor.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL3NlbnNvcnMvYmFzaF9zZW5zb3IucHk=) | `93.93% <100%> (-0.18%)` | :arrow_down: |
| [...ib/operators/oracle\_to\_azure\_data\_lake\_transfer.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL29wZXJhdG9ycy9vcmFjbGVfdG9fYXp1cmVfZGF0YV9sYWtlX3RyYW5zZmVyLnB5) | `97.72% <100%> (ø)` | :arrow_up: |
| [airflow/operators/docker\_operator.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9vcGVyYXRvcnMvZG9ja2VyX29wZXJhdG9yLnB5) | `95.78% <100%> (ø)` | :arrow_up: |
| [airflow/utils/file.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy91dGlscy9maWxlLnB5) | `86.3% <20%> (-5.17%)` | :arrow_down: |
| [airflow/kubernetes/volume\_mount.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3ZvbHVtZV9tb3VudC5weQ==) | `44.44% <0%> (-55.56%)` | :arrow_down: |
| ... and [5 more](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/6957?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/6957?src=pr&el=footer). Last update [5e100ab...10ecab2](https://codecov.io/gh/apache/airflow/pull/6957?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] codecov-io edited a comment on issue #6957:
[AIRFLOW-6396] Use tempfile.TemporaryDirectory instead of custom one
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #6957: [AIRFLOW-6396] Use tempfile.TemporaryDirectory instead of custom one
URL: https://github.com/apache/airflow/pull/6957#issuecomment-569559227
# [Codecov](https://codecov.io/gh/apache/airflow/pull/6957?src=pr&el=h1) Report
> Merging [#6957](https://codecov.io/gh/apache/airflow/pull/6957?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/0f7c4563d2f8885e12312aae72bc70a22f734894?src=pr&el=desc) will **decrease** coverage by `0.29%`.
> The diff coverage is `69.23%`.
[](https://codecov.io/gh/apache/airflow/pull/6957?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6957 +/- ##
========================================
- Coverage 84.7% 84.4% -0.3%
========================================
Files 679 679
Lines 38489 38475 -14
========================================
- Hits 32601 32474 -127
- Misses 5888 6001 +113
```
| [Impacted Files](https://codecov.io/gh/apache/airflow/pull/6957?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [airflow/hooks/pig\_hook.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9ob29rcy9waWdfaG9vay5weQ==) | `95.34% <100%> (-0.11%)` | :arrow_down: |
| [airflow/operators/bash\_operator.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9vcGVyYXRvcnMvYmFzaF9vcGVyYXRvci5weQ==) | `90.56% <100%> (-0.18%)` | :arrow_down: |
| [airflow/operators/s3\_to\_hive\_operator.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9vcGVyYXRvcnMvczNfdG9faGl2ZV9vcGVyYXRvci5weQ==) | `93.96% <100%> (-0.06%)` | :arrow_down: |
| [airflow/hooks/hive\_hooks.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9ob29rcy9oaXZlX2hvb2tzLnB5) | `77.55% <100%> (-0.06%)` | :arrow_down: |
| [airflow/operators/python\_operator.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9vcGVyYXRvcnMvcHl0aG9uX29wZXJhdG9yLnB5) | `95.8% <100%> (ø)` | :arrow_up: |
| [airflow/contrib/sensors/bash\_sensor.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL3NlbnNvcnMvYmFzaF9zZW5zb3IucHk=) | `93.93% <100%> (-0.18%)` | :arrow_down: |
| [...ib/operators/oracle\_to\_azure\_data\_lake\_transfer.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL29wZXJhdG9ycy9vcmFjbGVfdG9fYXp1cmVfZGF0YV9sYWtlX3RyYW5zZmVyLnB5) | `97.72% <100%> (ø)` | :arrow_up: |
| [airflow/operators/docker\_operator.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9vcGVyYXRvcnMvZG9ja2VyX29wZXJhdG9yLnB5) | `95.78% <100%> (ø)` | :arrow_up: |
| [airflow/utils/file.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy91dGlscy9maWxlLnB5) | `86.3% <20%> (-5.17%)` | :arrow_down: |
| [airflow/kubernetes/volume\_mount.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3ZvbHVtZV9tb3VudC5weQ==) | `44.44% <0%> (-55.56%)` | :arrow_down: |
| ... and [4 more](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/6957?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/6957?src=pr&el=footer). Last update [0f7c456...26a54ba](https://codecov.io/gh/apache/airflow/pull/6957?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on a change in pull request #6957:
[AIRFLOW-6396] Use tempfile.TemporaryDirectory instead of custom one
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #6957: [AIRFLOW-6396] Use tempfile.TemporaryDirectory instead of custom one
URL: https://github.com/apache/airflow/pull/6957#discussion_r361879711
##########
File path: UPDATING.md
##########
@@ -41,6 +41,10 @@ assists users migrating to a new version.
## Airflow Master
+### Remove airflow.utils.file.TemporaryDirectory
+
+Removing this because the same functionality is provided by `tempfile.TemporaryDirectory`.
Review comment:
No. This is based on my experience. I would expect the following points, so I try to include 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
With regards,
Apache Git Services
[GitHub] [airflow] nuclearpinguin commented on a change in pull request
#6957: [AIRFLOW-6396] Use tempfile.TemporaryDirectory instead of custom one
Posted by GitBox <gi...@apache.org>.
nuclearpinguin commented on a change in pull request #6957: [AIRFLOW-6396] Use tempfile.TemporaryDirectory instead of custom one
URL: https://github.com/apache/airflow/pull/6957#discussion_r361879507
##########
File path: UPDATING.md
##########
@@ -41,6 +41,10 @@ assists users migrating to a new version.
## Airflow Master
+### Remove airflow.utils.file.TemporaryDirectory
+
+Removing this because the same functionality is provided by `tempfile.TemporaryDirectory`.
Review comment:
Are those bullets points somewhere available?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] nuclearpinguin commented on a change in pull request
#6957: [AIRFLOW-6396] Use tempfile.TemporaryDirectory instead of custom one
Posted by GitBox <gi...@apache.org>.
nuclearpinguin commented on a change in pull request #6957: [AIRFLOW-6396] Use tempfile.TemporaryDirectory instead of custom one
URL: https://github.com/apache/airflow/pull/6957#discussion_r361879858
##########
File path: airflow/utils/file.py
##########
@@ -17,32 +17,14 @@
# specific language governing permissions and limitations
# under the License.
-import errno
import os
import re
-import shutil
import zipfile
-from contextlib import contextmanager
-from tempfile import mkdtemp
from typing import Dict, List, Optional, Pattern
from airflow import LoggingMixin, conf
Review comment:
I added deprecation warning.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] codecov-io edited a comment on issue #6957:
[AIRFLOW-6396] Use tempfile.TemporaryDirectory instead of custom one
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #6957: [AIRFLOW-6396] Use tempfile.TemporaryDirectory instead of custom one
URL: https://github.com/apache/airflow/pull/6957#issuecomment-569559227
# [Codecov](https://codecov.io/gh/apache/airflow/pull/6957?src=pr&el=h1) Report
> Merging [#6957](https://codecov.io/gh/apache/airflow/pull/6957?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/0f7c4563d2f8885e12312aae72bc70a22f734894?src=pr&el=desc) will **decrease** coverage by `0.29%`.
> The diff coverage is `69.23%`.
[](https://codecov.io/gh/apache/airflow/pull/6957?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6957 +/- ##
========================================
- Coverage 84.7% 84.4% -0.3%
========================================
Files 679 679
Lines 38489 38475 -14
========================================
- Hits 32601 32474 -127
- Misses 5888 6001 +113
```
| [Impacted Files](https://codecov.io/gh/apache/airflow/pull/6957?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [airflow/hooks/pig\_hook.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9ob29rcy9waWdfaG9vay5weQ==) | `95.34% <100%> (-0.11%)` | :arrow_down: |
| [airflow/operators/bash\_operator.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9vcGVyYXRvcnMvYmFzaF9vcGVyYXRvci5weQ==) | `90.56% <100%> (-0.18%)` | :arrow_down: |
| [airflow/operators/s3\_to\_hive\_operator.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9vcGVyYXRvcnMvczNfdG9faGl2ZV9vcGVyYXRvci5weQ==) | `93.96% <100%> (-0.06%)` | :arrow_down: |
| [airflow/hooks/hive\_hooks.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9ob29rcy9oaXZlX2hvb2tzLnB5) | `77.55% <100%> (-0.06%)` | :arrow_down: |
| [airflow/operators/python\_operator.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9vcGVyYXRvcnMvcHl0aG9uX29wZXJhdG9yLnB5) | `95.8% <100%> (ø)` | :arrow_up: |
| [airflow/contrib/sensors/bash\_sensor.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL3NlbnNvcnMvYmFzaF9zZW5zb3IucHk=) | `93.93% <100%> (-0.18%)` | :arrow_down: |
| [...ib/operators/oracle\_to\_azure\_data\_lake\_transfer.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL29wZXJhdG9ycy9vcmFjbGVfdG9fYXp1cmVfZGF0YV9sYWtlX3RyYW5zZmVyLnB5) | `97.72% <100%> (ø)` | :arrow_up: |
| [airflow/operators/docker\_operator.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9vcGVyYXRvcnMvZG9ja2VyX29wZXJhdG9yLnB5) | `95.78% <100%> (ø)` | :arrow_up: |
| [airflow/utils/file.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy91dGlscy9maWxlLnB5) | `86.3% <20%> (-5.17%)` | :arrow_down: |
| [airflow/kubernetes/volume\_mount.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3ZvbHVtZV9tb3VudC5weQ==) | `44.44% <0%> (-55.56%)` | :arrow_down: |
| ... and [4 more](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/6957?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/6957?src=pr&el=footer). Last update [0f7c456...26a54ba](https://codecov.io/gh/apache/airflow/pull/6957?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] nuclearpinguin commented on a change in pull request
#6957: [AIRFLOW-6396] Use tempfile.TemporaryDirectory instead of custom one
Posted by GitBox <gi...@apache.org>.
nuclearpinguin commented on a change in pull request #6957: [AIRFLOW-6396] Use tempfile.TemporaryDirectory instead of custom one
URL: https://github.com/apache/airflow/pull/6957#discussion_r361879887
##########
File path: UPDATING.md
##########
@@ -41,6 +41,10 @@ assists users migrating to a new version.
## Airflow Master
+### Remove airflow.utils.file.TemporaryDirectory
+
+Removing this because the same functionality is provided by `tempfile.TemporaryDirectory`.
Review comment:
Maybe it's worth to add them to 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on a change in pull request #6957:
[AIRFLOW-6396] Use tempfile.TemporaryDirectory instead of custom one
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #6957: [AIRFLOW-6396] Use tempfile.TemporaryDirectory instead of custom one
URL: https://github.com/apache/airflow/pull/6957#discussion_r361879362
##########
File path: UPDATING.md
##########
@@ -41,6 +41,10 @@ assists users migrating to a new version.
## Airflow Master
+### Remove airflow.utils.file.TemporaryDirectory
+
+Removing this because the same functionality is provided by `tempfile.TemporaryDirectory`.
Review comment:
Can you add more information here?
In my opinion, a good note should contain the following information:
- [ ] Previous behaviors
- [ ] New behaviors
- [ ] If possible, a simple example of how to migrate
- [ ] If possible, the benefit for the user after migration e.g. "we want to make these changes to unify class names."
- [ ] If possible, the reason for the change e.g. reference for AIP-21
Here is a similar example that includes most if not all of the requirements:
https://github.com/apache/airflow/blob/master/UPDATING.md#removal-of-redirect_stdout-redirect_stderr
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] nuclearpinguin merged pull request #6957: [AIRFLOW-6396]
Use tempfile.TemporaryDirectory instead of custom one
Posted by GitBox <gi...@apache.org>.
nuclearpinguin merged pull request #6957: [AIRFLOW-6396] Use tempfile.TemporaryDirectory instead of custom one
URL: https://github.com/apache/airflow/pull/6957
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] codecov-io commented on issue #6957: [AIRFLOW-6396] Use
tempfile.TemporaryDirectory instead of custom one
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #6957: [AIRFLOW-6396] Use tempfile.TemporaryDirectory instead of custom one
URL: https://github.com/apache/airflow/pull/6957#issuecomment-569559227
# [Codecov](https://codecov.io/gh/apache/airflow/pull/6957?src=pr&el=h1) Report
> Merging [#6957](https://codecov.io/gh/apache/airflow/pull/6957?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/5e100ab753635521d33f287bdffd3d9bd4c6ae0f?src=pr&el=desc) will **decrease** coverage by `0.3%`.
> The diff coverage is `69.23%`.
[](https://codecov.io/gh/apache/airflow/pull/6957?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6957 +/- ##
==========================================
- Coverage 84.7% 84.39% -0.31%
==========================================
Files 679 679
Lines 38488 38474 -14
==========================================
- Hits 32600 32472 -128
- Misses 5888 6002 +114
```
| [Impacted Files](https://codecov.io/gh/apache/airflow/pull/6957?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [airflow/hooks/pig\_hook.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9ob29rcy9waWdfaG9vay5weQ==) | `95.34% <100%> (-0.11%)` | :arrow_down: |
| [airflow/operators/bash\_operator.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9vcGVyYXRvcnMvYmFzaF9vcGVyYXRvci5weQ==) | `90.38% <100%> (-0.19%)` | :arrow_down: |
| [airflow/operators/s3\_to\_hive\_operator.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9vcGVyYXRvcnMvczNfdG9faGl2ZV9vcGVyYXRvci5weQ==) | `93.96% <100%> (-0.06%)` | :arrow_down: |
| [airflow/hooks/hive\_hooks.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9ob29rcy9oaXZlX2hvb2tzLnB5) | `77.55% <100%> (-0.06%)` | :arrow_down: |
| [airflow/operators/python\_operator.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9vcGVyYXRvcnMvcHl0aG9uX29wZXJhdG9yLnB5) | `95.8% <100%> (ø)` | :arrow_up: |
| [airflow/contrib/sensors/bash\_sensor.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL3NlbnNvcnMvYmFzaF9zZW5zb3IucHk=) | `93.93% <100%> (-0.18%)` | :arrow_down: |
| [...ib/operators/oracle\_to\_azure\_data\_lake\_transfer.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL29wZXJhdG9ycy9vcmFjbGVfdG9fYXp1cmVfZGF0YV9sYWtlX3RyYW5zZmVyLnB5) | `97.72% <100%> (ø)` | :arrow_up: |
| [airflow/operators/docker\_operator.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9vcGVyYXRvcnMvZG9ja2VyX29wZXJhdG9yLnB5) | `95.78% <100%> (ø)` | :arrow_up: |
| [airflow/utils/file.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy91dGlscy9maWxlLnB5) | `86.3% <20%> (-5.17%)` | :arrow_down: |
| [airflow/kubernetes/volume\_mount.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3ZvbHVtZV9tb3VudC5weQ==) | `44.44% <0%> (-55.56%)` | :arrow_down: |
| ... and [5 more](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/6957?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/6957?src=pr&el=footer). Last update [5e100ab...10ecab2](https://codecov.io/gh/apache/airflow/pull/6957?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on a change in pull request #6957:
[AIRFLOW-6396] Use tempfile.TemporaryDirectory instead of custom one
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #6957: [AIRFLOW-6396] Use tempfile.TemporaryDirectory instead of custom one
URL: https://github.com/apache/airflow/pull/6957#discussion_r361879497
##########
File path: airflow/utils/file.py
##########
@@ -17,32 +17,14 @@
# specific language governing permissions and limitations
# under the License.
-import errno
import os
import re
-import shutil
import zipfile
-from contextlib import contextmanager
-from tempfile import mkdtemp
from typing import Dict, List, Optional, Pattern
from airflow import LoggingMixin, conf
Review comment:
We can shorten this path if we suspect that users do not use this function, but I think that this does not apply in this case.. This PR introduces changes to 13 files, so this is a commonly used function.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on a change in pull request #6957:
[AIRFLOW-6396] Use tempfile.TemporaryDirectory instead of custom one
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #6957: [AIRFLOW-6396] Use tempfile.TemporaryDirectory instead of custom one
URL: https://github.com/apache/airflow/pull/6957#discussion_r361881067
##########
File path: UPDATING.md
##########
@@ -41,6 +41,10 @@ assists users migrating to a new version.
## Airflow Master
+### Remove airflow.utils.file.TemporaryDirectory
+
+Removing this because the same functionality is provided by `tempfile.TemporaryDirectory`.
Review comment:
https://github.com/apache/airflow/pull/6960
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] nuclearpinguin commented on a change in pull request
#6957: [AIRFLOW-6396] Use tempfile.TemporaryDirectory instead of custom one
Posted by GitBox <gi...@apache.org>.
nuclearpinguin commented on a change in pull request #6957: [AIRFLOW-6396] Use tempfile.TemporaryDirectory instead of custom one
URL: https://github.com/apache/airflow/pull/6957#discussion_r361929911
##########
File path: UPDATING.md
##########
@@ -41,6 +41,10 @@ assists users migrating to a new version.
## Airflow Master
+### Deprecated airflow.utils.file.TemporaryDirectory
+
+Instead user should use `tempfile.TemporaryDirectory` which provides the same functionality.
Review comment:
I think now the description is sufficient.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on a change in pull request #6957:
[AIRFLOW-6396] Use tempfile.TemporaryDirectory instead of custom one
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #6957: [AIRFLOW-6396] Use tempfile.TemporaryDirectory instead of custom one
URL: https://github.com/apache/airflow/pull/6957#discussion_r361879124
##########
File path: airflow/utils/file.py
##########
@@ -17,32 +17,14 @@
# specific language governing permissions and limitations
# under the License.
-import errno
import os
import re
-import shutil
import zipfile
-from contextlib import contextmanager
-from tempfile import mkdtemp
from typing import Dict, List, Optional, Pattern
from airflow import LoggingMixin, conf
Review comment:
Can you maintain backward compatibility to make migrations easier for users? I think I don't need to take drastic steps and immediately delete this function without warning. I think that changes should always be done in several stages:
- initial version
- initial version with a deprecation warning
- remove features.
We can make changes to Airflow 2.0 that are not backward compatible, but these should be individual exceptions, but not the only way to make changes.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on a change in pull request #6957:
[AIRFLOW-6396] Use tempfile.TemporaryDirectory instead of custom one
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #6957: [AIRFLOW-6396] Use tempfile.TemporaryDirectory instead of custom one
URL: https://github.com/apache/airflow/pull/6957#discussion_r361882019
##########
File path: UPDATING.md
##########
@@ -41,6 +41,10 @@ assists users migrating to a new version.
## Airflow Master
+### Deprecated airflow.utils.file.TemporaryDirectory
+
+Instead user should use `tempfile.TemporaryDirectory` which provides the same functionality.
Review comment:
Can you add an example?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] codecov-io edited a comment on issue #6957:
[AIRFLOW-6396] Use tempfile.TemporaryDirectory instead of custom one
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #6957: [AIRFLOW-6396] Use tempfile.TemporaryDirectory instead of custom one
URL: https://github.com/apache/airflow/pull/6957#issuecomment-569559227
# [Codecov](https://codecov.io/gh/apache/airflow/pull/6957?src=pr&el=h1) Report
> Merging [#6957](https://codecov.io/gh/apache/airflow/pull/6957?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/5e100ab753635521d33f287bdffd3d9bd4c6ae0f?src=pr&el=desc) will **decrease** coverage by `0.3%`.
> The diff coverage is `69.23%`.
[](https://codecov.io/gh/apache/airflow/pull/6957?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6957 +/- ##
==========================================
- Coverage 84.7% 84.39% -0.31%
==========================================
Files 679 679
Lines 38488 38474 -14
==========================================
- Hits 32600 32472 -128
- Misses 5888 6002 +114
```
| [Impacted Files](https://codecov.io/gh/apache/airflow/pull/6957?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [airflow/hooks/pig\_hook.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9ob29rcy9waWdfaG9vay5weQ==) | `95.34% <100%> (-0.11%)` | :arrow_down: |
| [airflow/operators/bash\_operator.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9vcGVyYXRvcnMvYmFzaF9vcGVyYXRvci5weQ==) | `90.38% <100%> (-0.19%)` | :arrow_down: |
| [airflow/operators/s3\_to\_hive\_operator.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9vcGVyYXRvcnMvczNfdG9faGl2ZV9vcGVyYXRvci5weQ==) | `93.96% <100%> (-0.06%)` | :arrow_down: |
| [airflow/hooks/hive\_hooks.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9ob29rcy9oaXZlX2hvb2tzLnB5) | `77.55% <100%> (-0.06%)` | :arrow_down: |
| [airflow/operators/python\_operator.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9vcGVyYXRvcnMvcHl0aG9uX29wZXJhdG9yLnB5) | `95.8% <100%> (ø)` | :arrow_up: |
| [airflow/contrib/sensors/bash\_sensor.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL3NlbnNvcnMvYmFzaF9zZW5zb3IucHk=) | `93.93% <100%> (-0.18%)` | :arrow_down: |
| [...ib/operators/oracle\_to\_azure\_data\_lake\_transfer.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL29wZXJhdG9ycy9vcmFjbGVfdG9fYXp1cmVfZGF0YV9sYWtlX3RyYW5zZmVyLnB5) | `97.72% <100%> (ø)` | :arrow_up: |
| [airflow/operators/docker\_operator.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9vcGVyYXRvcnMvZG9ja2VyX29wZXJhdG9yLnB5) | `95.78% <100%> (ø)` | :arrow_up: |
| [airflow/utils/file.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy91dGlscy9maWxlLnB5) | `86.3% <20%> (-5.17%)` | :arrow_down: |
| [airflow/kubernetes/volume\_mount.py](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3ZvbHVtZV9tb3VudC5weQ==) | `44.44% <0%> (-55.56%)` | :arrow_down: |
| ... and [5 more](https://codecov.io/gh/apache/airflow/pull/6957/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/6957?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/6957?src=pr&el=footer). Last update [5e100ab...10ecab2](https://codecov.io/gh/apache/airflow/pull/6957?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on a change in pull request #6957:
[AIRFLOW-6396] Use tempfile.TemporaryDirectory instead of custom one
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #6957: [AIRFLOW-6396] Use tempfile.TemporaryDirectory instead of custom one
URL: https://github.com/apache/airflow/pull/6957#discussion_r361879124
##########
File path: airflow/utils/file.py
##########
@@ -17,32 +17,14 @@
# specific language governing permissions and limitations
# under the License.
-import errno
import os
import re
-import shutil
import zipfile
-from contextlib import contextmanager
-from tempfile import mkdtemp
from typing import Dict, List, Optional, Pattern
from airflow import LoggingMixin, conf
Review comment:
Can you maintain backward compatibility to make migrations easier for users? I think I don't need to take drastic steps and immediately delete this function without warning. I think that changes should always be done in several stages:
-initial version
- initial version with a deprecation warning
- remove features.
We can make changes to Airflow 2.0 that are not backward compatible, but these should be individual exceptions, but not the only way to make changes.
----------------------------------------------------------------
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
With regards,
Apache Git Services