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/09/01 13:45:32 UTC
[GitHub] [airflow] kokorin opened a new pull request, #26105: Fix: git clone on Windows
kokorin opened a new pull request, #26105:
URL: https://github.com/apache/airflow/pull/26105
Airflow repository can't be cloned on Windows due to some files containing symbols restricted on Windows.
closes: #24596
---
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 changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+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 a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
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 #26105: Fix: git clone on Windows
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #26105:
URL: https://github.com/apache/airflow/pull/26105#issuecomment-1236767676
@uranusjr - WDYT?
--
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 #26105: Fix: git clone on Windows
Posted by GitBox <gi...@apache.org>.
uranusjr commented on PR #26105:
URL: https://github.com/apache/airflow/pull/26105#issuecomment-1238896341
The CI images job is failing for some other PRs as well, so perhaps it is related to some issues in GitHub. I have restarted the job to see if the issue is temporary.
--
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 #26105: Fix: git clone on Windows
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #26105:
URL: https://github.com/apache/airflow/pull/26105#issuecomment-1239584515
> The CI images job is failing for some other PRs as well, so perhaps it is related to some issues in GitHub. I have restarted the job to see if the issue is temporary.
Yeah. I think GitHub had a rough day yesterday ... Many things were failing here and there
--
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 #26105: Fix: git clone on Windows
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #26105:
URL: https://github.com/apache/airflow/pull/26105#issuecomment-1236767181
Very nice approach :).
--
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 #26105: Fix: git clone on Windows
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #26105:
URL: https://github.com/apache/airflow/pull/26105#issuecomment-1238558439
> @potiuk sorry but my first question was if the tests in question were flaky. The same tests fail in #26162. In both PR tests fail inside breeze: `airflow_breeze/commands/kubernetes_commands.py` 
What I mean, if you already did points 1) 2) 3) - just mention it and when you ask if this is a failure just point to the other similar issues :) . Try to put. your self in our shoes. When you ask such question and you do not mention that you already looked at it, the first assumption I have is .. that you are lazy and you have not checked just try to offload the check to us :).
No judgment here, just imagine how it looks like:
a) @potiuk @uranus - should I fix it ?
vs.
b) It looks like the error here is similar to error in #26162 , and it does not loook related to my changes. @potiuk @uranusjr - can you help please?
The first looks like you have not made any effort to check and you are lazy. The second shows that you've "done your part". Context is everything. My reaction to b) is vastly different than b) :).
Hope no hard feelings - just wantted to show you how it looks like if you are too brief.
--
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 #26105: Fix: git clone on Windows
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #26105:
URL: https://github.com/apache/airflow/pull/26105#issuecomment-1235316267
> My proposal is not to make FileTaskHandler work on Windows, only to modify it in a way that is in line to the changes we want to make _if we want to make it work_. This makes the work more meaningful; the current patch you proposes will basically all be deleted when Windows support is implemented, which is more wasteful than just implement the proper logic in the first place.
Yeah. Agree with @uranusjr that this is the right way. Regarding generation of the files, it might even be as simple as zipping the log files that are used for test and have two .zip files "windows" / "posix" - and decompress the one that is relevant for the OS that you are running the tests on. This way you could add the different behaviours in FileTaskHandler too.
--
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] kokorin commented on pull request #26105: Fix: git clone on Windows
Posted by GitBox <gi...@apache.org>.
kokorin commented on PR #26105:
URL: https://github.com/apache/airflow/pull/26105#issuecomment-1245151373
@uranusjr @potiuk do you think this PR can be merged now? The only test failures are related to help and (probably) are flaky. The only failed static is related to documentation ("a/docs/apache-airflow/img/airflow_erd.sha256") and changes this PR should not be the reason of its failure.
--
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] kokorin commented on pull request #26105: Fix: git clone on Windows
Posted by GitBox <gi...@apache.org>.
kokorin commented on PR #26105:
URL: https://github.com/apache/airflow/pull/26105#issuecomment-1238457279
@potiuk sorry but my first question was if the tests in question were flaky. The same tests fail in #26162.
In both PR tests fail inside breeze: `airflow_breeze/commands/kubernetes_commands.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] uranusjr commented on a diff in pull request #26105: Fix: git clone on Windows
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #26105:
URL: https://github.com/apache/airflow/pull/26105#discussion_r964043467
##########
tests/www/views/test_views_log.py:
##########
@@ -163,6 +169,25 @@ def tis(dags, session):
clear_db_runs()
+@pytest.fixture
+def create_expected_log_file(log_path, tis):
+ ti, _ = tis
+ handler = FileTaskHandler(log_path)
+
+ def create_expected_log_file(try_number):
+ ti.try_number = try_number - 1
+ handler.set_context(ti)
+ handler.emit(logging.makeLogRecord({"msg": "Log for testing."}))
+ handler.flush()
+
+ yield create_expected_log_file
+ # log_path fixture has scope "module" because it is used in log_app which has module scope, so
+ # log_path isn't deleted automatically by pytest between tests
+ # we delete created log files manually to make sure tests do not reuse logs created by other tests
+ for sub_path in log_path.glob("*"):
Review Comment:
```suggestion
for sub_path in log_path.iterdir():
```
Should be equivalent…?
--
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 diff in pull request #26105: Fix: git clone on Windows
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #26105:
URL: https://github.com/apache/airflow/pull/26105#discussion_r963492911
##########
tests/www/views/test_views_log.py:
##########
@@ -163,6 +169,22 @@ def tis(dags, session):
clear_db_runs()
+@pytest.fixture
+def create_expected_log_file(log_path, tis):
+ ti, _ = tis
+ handler = FileTaskHandler(log_path)
+
+ def create_expected_log_file(try_number):
+ ti.try_number = try_number - 1
+ handler.set_context(ti)
+ handler.emit(logging.makeLogRecord({"msg": "Log for testing."}))
+ handler.flush()
+
+ yield create_expected_log_file
+ # delete logs to minimize tests interference
+ shutil.rmtree(log_path)
Review Comment:
That makes sense, thanks. Would it be a good idea to only delete the generated log file instead though? (And add a comment explaining why)
--
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 #26105: Fix: git clone on Windows
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #26105:
URL: https://github.com/apache/airflow/pull/26105#issuecomment-1238046386
> @potiuk @uranusjr 3 Helm tests failed during last CI run, are they flaky or should I fix them first?
@kokrin - your goal is to make it green. And whether to do it is something that you shoud be able to assess yourself I think
1) assume it's you first and look at the actual log/error if you can figure if it is related
2) if this is a once-off failure among multiple similar jobs and seems unrelated/networking problem - commit --amend, push and check if it is reproduced
3) if other PRs and main are failing with similar problems - assume it is a shared/main problem and raise awareness (here or in #development slack)
3) it the log seems releated and failure repeats in several similar jobs - reproduce locally and fix (the K8S tests have now very nice, reproducible path that you can run locally - see the exact recipe in TESTING.rst
4) if you are lost after doing all the above - ping us :)
I think this is a good recipe and I wonder if following it will help in this case (I am using this a bit of a testing ground and if it works I will try to make contributors aware of this "approach" they should use - I am looking for a ways to scale the PR process better. Let me know @kokorin - and maybe if you have other suggestions how to improve the "process" that woudl be awesome.
--
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] kokorin commented on pull request #26105: Fix: git clone on Windows
Posted by GitBox <gi...@apache.org>.
kokorin commented on PR #26105:
URL: https://github.com/apache/airflow/pull/26105#issuecomment-1234736082
@potiuk after investigation it looks like the root of the problem is in [FileTaskHandler](https://github.com/apache/airflow/blob/main/airflow/utils/log/file_task_handler.py#L115) as it writes logs to paths containing dates in [ISO 8601format](https://en.wikipedia.org/wiki/ISO_8601). But ISO 8601format won't work on Windows.
[Rewrite Breeze and CI in Python ](https://github.com/apache/airflow/projects/13) project has no issue to use another date format to make log directory structure Windows-compatible. Probably it's because the project aimed at Breeze rewriting, not on been able to run Airflow on Windows.
Changing `FileTaskHandler` is out of scope of this PR and the only way I see is to add a class extending `FileTaskHandler` and overwriting log directory structure with Windows compatible. After that `DEFAULT_LOGGING_CONFIG` in test should be adjusted to use customized `FileTaskHandler`.
The class (customized `FileTaskHandler`) itself is not a problem, but I'm not an expert in Python and newbie Airflow developer. Could you tell me how to make Airflow/Breeze to load customized `FileTaskHandler`?
--
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 #26105: Fix: git clone on Windows
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #26105:
URL: https://github.com/apache/airflow/pull/26105#issuecomment-1241433859
Rebased to re-run.
--
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] kokorin commented on pull request #26105: Fix: git clone on Windows
Posted by GitBox <gi...@apache.org>.
kokorin commented on PR #26105:
URL: https://github.com/apache/airflow/pull/26105#issuecomment-1238886977
CI steps "Build CI Images" fail with
```
#7 [internal] load metadata for docker.io/library/python:3.10-slim-bullseye
------
> pushing ghcr.io/apache/airflow/main/ci/python3.10:71f9e1d391969806a7ec44bbb2449072b655b6d7 with docker:
------
error: denied: permission_denied: write_package
```
Probably, it's not related to changes in this MR. Please, let me know if it's not true.
--
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] kokorin commented on pull request #26105: Fix: git clone on Windows
Posted by GitBox <gi...@apache.org>.
kokorin commented on PR #26105:
URL: https://github.com/apache/airflow/pull/26105#issuecomment-1235232684
@uranusjr I have came to an idea that to fix git clone on Windows it would be enough to fix points 2 and 3:
>2. Delete those problematic files from the repository, and general those files dynamically when testing (under appropriate file names) instead in Pytest’s tmp_path.
>3. Modify tests to appropriately point the log directory to tmp_path.
While point 1 could be fixed in another PR:
> 1. Fix the actual `FileTaskHandler` to log in different a file name format in Windows.
--
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] kokorin commented on a diff in pull request #26105: Fix: git clone on Windows
Posted by GitBox <gi...@apache.org>.
kokorin commented on code in PR #26105:
URL: https://github.com/apache/airflow/pull/26105#discussion_r964332641
##########
tests/www/views/test_views_log.py:
##########
@@ -163,6 +169,25 @@ def tis(dags, session):
clear_db_runs()
+@pytest.fixture
+def create_expected_log_file(log_path, tis):
+ ti, _ = tis
+ handler = FileTaskHandler(log_path)
+
+ def create_expected_log_file(try_number):
+ ti.try_number = try_number - 1
+ handler.set_context(ti)
+ handler.emit(logging.makeLogRecord({"msg": "Log for testing."}))
+ handler.flush()
+
+ yield create_expected_log_file
+ # log_path fixture has scope "module" because it is used in log_app which has module scope, so
+ # log_path isn't deleted automatically by pytest between tests
+ # we delete created log files manually to make sure tests do not reuse logs created by other tests
+ for sub_path in log_path.glob("*"):
Review Comment:
applied
--
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 #26105: Fix: git clone on Windows
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #26105:
URL: https://github.com/apache/airflow/pull/26105#issuecomment-1250395896
> @uranusjr @potiuk do you think this PR can be merged now? The only test failures are related to help and (probably) are flaky. The only failed static is related to documentation ("a/docs/apache-airflow/img/airflow_erd.sha256") and changes this PR should not be the reason of its failure.
In such cases, you can always rebase the PR to check (it's as easy as choosing "rebase" at the bottom right of the PR if there are no conflicts). I did it - but next time you can rebase yourself without waiting/pinging us (let's see if it succeed - please watch this and check and ping me if it does, otherwise I will come back to this one next time when I go through about 100/200 mails so it might take some time).
--
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 #26105: Fix: git clone on Windows
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on PR #26105:
URL: https://github.com/apache/airflow/pull/26105#issuecomment-1250878586
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] potiuk commented on pull request #26105: Fix: git clone on Windows
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #26105:
URL: https://github.com/apache/airflow/pull/26105#issuecomment-1234630135
Good idea to add a test for it :). But you will also need to fix some tests to make it works I am afraid.
BTW. That will be really helpful (and should be part of [#windows](https://github.com/apache/airflow/issues/10388) - which I tihnk very soon (pending few more PRs to complete https://github.com/apache/airflow/projects/13 ) will finally be possible.
--
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 #26105: Fix: git clone on Windows
Posted by GitBox <gi...@apache.org>.
uranusjr commented on PR #26105:
URL: https://github.com/apache/airflow/pull/26105#issuecomment-1235248798
My proposal is not to make FileTaskHandler work on Windows, only to modify it in a way that is in line to the changes we want to make _if we want to make it work_. This makes the work more meaningful; the current patch you proposes will basically all be deleted when Windows support is implemented, which is more wasteful than just implement the proper logic in the first place.
--
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] kokorin commented on pull request #26105: Fix: git clone on Windows
Posted by GitBox <gi...@apache.org>.
kokorin commented on PR #26105:
URL: https://github.com/apache/airflow/pull/26105#issuecomment-1235203848
> 1. Fix the actual `FileTaskHandler` to log in different a file name format in Windows.
@uranusjr I like your suggestion, the only problem I see is that it's not possible to run `FileTaskHandler` tests on Windows before fixing all other Windows-related issues.
So if it's ok to have in `FileTaskHandler` windows-related code which is not tested on Windows - I will work on that.
--
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] kokorin commented on pull request #26105: Fix: git clone on Windows
Posted by GitBox <gi...@apache.org>.
kokorin commented on PR #26105:
URL: https://github.com/apache/airflow/pull/26105#issuecomment-1250864660
@potiuk all tests passed
--
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 #26105: Fix: git clone on Windows
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #26105:
URL: https://github.com/apache/airflow/pull/26105#issuecomment-1250878651
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.
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 #26105: Fix: git clone on Windows
Posted by GitBox <gi...@apache.org>.
potiuk merged PR #26105:
URL: https://github.com/apache/airflow/pull/26105
--
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 diff in pull request #26105: Fix: git clone on Windows
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #26105:
URL: https://github.com/apache/airflow/pull/26105#discussion_r963425906
##########
tests/www/views/test_views_log.py:
##########
@@ -163,6 +169,22 @@ def tis(dags, session):
clear_db_runs()
+@pytest.fixture
+def create_expected_log_file(log_path, tis):
+ ti, _ = tis
+ handler = FileTaskHandler(log_path)
+
+ def create_expected_log_file(try_number):
+ ti.try_number = try_number - 1
+ handler.set_context(ti)
+ handler.emit(logging.makeLogRecord({"msg": "Log for testing."}))
+ handler.flush()
+
+ yield create_expected_log_file
+ # delete logs to minimize tests interference
+ shutil.rmtree(log_path)
Review Comment:
Not needed, pytest tracks `tmp_path_factory` and cleans things it creates 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.
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 diff in pull request #26105: Fix: git clone on Windows
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #26105:
URL: https://github.com/apache/airflow/pull/26105#discussion_r963425906
##########
tests/www/views/test_views_log.py:
##########
@@ -163,6 +169,22 @@ def tis(dags, session):
clear_db_runs()
+@pytest.fixture
+def create_expected_log_file(log_path, tis):
+ ti, _ = tis
+ handler = FileTaskHandler(log_path)
+
+ def create_expected_log_file(try_number):
+ ti.try_number = try_number - 1
+ handler.set_context(ti)
+ handler.emit(logging.makeLogRecord({"msg": "Log for testing."}))
+ handler.flush()
+
+ yield create_expected_log_file
+ # delete logs to minimize tests interference
+ shutil.rmtree(log_path)
Review Comment:
Not needed, pytest cleans things in the temp directory 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.
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 #26105: Fix: git clone on Windows
Posted by GitBox <gi...@apache.org>.
uranusjr commented on PR #26105:
URL: https://github.com/apache/airflow/pull/26105#issuecomment-1235178164
I commented elsewhere. Since this actually affects Windows _support_ (which we want to achieve at some point), the root cause should also be fixed. I would suggest the following steps:
1. Fix the actual `FileTaskHandler` to log in different a file name format in Windows.
2. Delete those problematic files from the repository, and general those files dynamically when testing (under appropriate file names) instead in Pytest’s `tmp_path`.
3. Modify tests to appropriately point the log directory to `tmp_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.
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 #26105: Fix: git clone on Windows
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on PR #26105:
URL: https://github.com/apache/airflow/pull/26105#issuecomment-1234304060
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
[GitHub] [airflow] kokorin commented on pull request #26105: Fix: git clone on Windows
Posted by GitBox <gi...@apache.org>.
kokorin commented on PR #26105:
URL: https://github.com/apache/airflow/pull/26105#issuecomment-1234672823
> But you will also need to fix some tests to make it works I am afraid.
That's clear. I'm working on that.
--
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] kokorin commented on pull request #26105: Fix: git clone on Windows
Posted by GitBox <gi...@apache.org>.
kokorin commented on PR #26105:
URL: https://github.com/apache/airflow/pull/26105#issuecomment-1237826615
@potiuk @uranusjr 3 Helm tests failed during last CI run, are they flaky or should I fix them first?
--
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] kokorin commented on a diff in pull request #26105: Fix: git clone on Windows
Posted by GitBox <gi...@apache.org>.
kokorin commented on code in PR #26105:
URL: https://github.com/apache/airflow/pull/26105#discussion_r963447265
##########
tests/www/views/test_views_log.py:
##########
@@ -163,6 +169,22 @@ def tis(dags, session):
clear_db_runs()
+@pytest.fixture
+def create_expected_log_file(log_path, tis):
+ ti, _ = tis
+ handler = FileTaskHandler(log_path)
+
+ def create_expected_log_file(try_number):
+ ti.try_number = try_number - 1
+ handler.set_context(ti)
+ handler.emit(logging.makeLogRecord({"msg": "Log for testing."}))
+ handler.flush()
+
+ yield create_expected_log_file
+ # delete logs to minimize tests interference
+ shutil.rmtree(log_path)
Review Comment:
@uranusjr pytest cleans directories after exiting defined scope. `log_path` fixture requires `module` scope to be used by `log_app` fixture (which also has `module` scope). But `log_path` should be cleared after each test, because almost all tests reference to the same Dag.
--
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] kokorin commented on a diff in pull request #26105: Fix: git clone on Windows
Posted by GitBox <gi...@apache.org>.
kokorin commented on code in PR #26105:
URL: https://github.com/apache/airflow/pull/26105#discussion_r963503623
##########
tests/www/views/test_views_log.py:
##########
@@ -163,6 +169,22 @@ def tis(dags, session):
clear_db_runs()
+@pytest.fixture
+def create_expected_log_file(log_path, tis):
+ ti, _ = tis
+ handler = FileTaskHandler(log_path)
+
+ def create_expected_log_file(try_number):
+ ti.try_number = try_number - 1
+ handler.set_context(ti)
+ handler.emit(logging.makeLogRecord({"msg": "Log for testing."}))
+ handler.flush()
+
+ yield create_expected_log_file
+ # delete logs to minimize tests interference
+ shutil.rmtree(log_path)
Review Comment:
@uranusjr in the PR I tried not to make any assumptions regarding log file name. I will add a comment describing why manual log file deletion is required and will find more clear to delete created log file.
--
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 #26105: Fix: git clone on Windows
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #26105:
URL: https://github.com/apache/airflow/pull/26105#issuecomment-1241433790
I fixed the K8S test problems in #26235 . 🤞
--
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] kokorin commented on pull request #26105: Fix: git clone on Windows
Posted by GitBox <gi...@apache.org>.
kokorin commented on PR #26105:
URL: https://github.com/apache/airflow/pull/26105#issuecomment-1239652436
@potiuk @uranusjr looking forward to merge :-)
--
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