Fix: git clone on Windows #26105

kokorin · 2022-09-01T13:45:29Z

Airflow repository can't be cloned on Windows due to some files containing symbols restricted on Windows.

closes: #24596
related to: #25865

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
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.

boring-cyborg · 2022-09-01T13:45:33Z

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 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 Consider adding an example DAG that shows how users should use it.
Consider using Breeze environment 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 for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
Be sure to read the Airflow Coding style.
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: [email protected]
Slack: https://s.apache.org/airflow-slack

potiuk · 2022-09-01T18:21:10Z

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 - which I tihnk very soon (pending few more PRs to complete https://github.com/apache/airflow/projects/13 ) will finally be possible.

kokorin · 2022-09-01T19:05:44Z

But you will also need to fix some tests to make it works I am afraid.

That's clear. I'm working on that.

kokorin · 2022-09-01T20:16:15Z

@potiuk after investigation it looks like the root of the problem is in FileTaskHandler as it writes logs to paths containing dates in ISO 8601format. But ISO 8601format won't work on Windows.

Rewrite Breeze and CI in Python 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 at 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 another class (in test) 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?

uranusjr · 2022-09-02T07:43:53Z

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:

Fix the actual FileTaskHandler to log in different a file name format in Windows.
Delete those problematic files from the repository, and general those files dynamically when testing (under appropriate file names) instead in Pytest’s tmp_path.
Modify tests to appropriately point the log directory to tmp_path.

kokorin · 2022-09-02T08:12:36Z

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.

kokorin · 2022-09-02T08:40:15Z

@uranusjr I have came to an idea that to fix git clone on Windows it would be enough to fix points 2 and 3:

Delete those problematic files from the repository, and general those files dynamically when testing (under appropriate file names) instead in Pytest’s tmp_path.

Modify tests to appropriately point the log directory to tmp_path.

While point 1 could be fixed in another PR:

Fix the actual FileTaskHandler to log in different a file name format in Windows.

uranusjr · 2022-09-02T08:57:30Z

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.

potiuk · 2022-09-02T10:08:05Z

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.

potiuk

LGTM

potiuk · 2022-09-05T09:36:12Z

Very nice approach :).

potiuk · 2022-09-05T09:36:37Z

@uranusjr - WDYT?

kokorin · 2022-09-06T08:28:29Z

@potiuk @uranusjr 3 Helm tests failed during last CI run, are they flaky or should I fix them first?

uranusjr · 2022-09-06T08:40:00Z

tests/www/views/test_views_log.py

+    # delete logs to minimize tests interference
+    shutil.rmtree(log_path)


Not needed, pytest tracks tmp_path_factory and cleans things it creates automatically.

@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.

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)

@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.

potiuk · 2022-09-06T11:56:49Z

@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

assume it's you first and look at the actual log/error if you can figure if it is related
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
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)
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
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.

kokorin · 2022-09-06T17:32:20Z

@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

tests/www/views/test_views_log.py

potiuk · 2022-09-06T19:20:39Z

@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 a) :).

Hope no hard feelings - just wantted to show you how it looks like if you are too brief.

kokorin · 2022-09-07T04:13:41Z

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.

uranusjr · 2022-09-07T04:32:07Z

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.

potiuk · 2022-09-07T15:58:05Z

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

kokorin · 2022-09-07T17:00:25Z

@potiuk @uranusjr looking forward to merge :-)

potiuk · 2022-09-09T02:41:16Z

I fixed the K8S test problems in #26235 . 🤞

potiuk · 2022-09-09T02:41:26Z

Rebased to re-run.

kokorin · 2022-09-13T09:35:36Z

@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.

potiuk · 2022-09-18T22:06:59Z

@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).

kokorin · 2022-09-19T10:54:00Z

@potiuk all tests passed

boring-cyborg · 2022-09-19T11:09:56Z

Awesome work, congrats on your first merged pull request!

potiuk · 2022-09-19T11:09:59Z

Cool!

(cherry picked from commit 9aa589e)

boring-cyborg bot added the area:dev-tools label Sep 1, 2022

kokorin marked this pull request as ready for review September 5, 2022 09:32

kokorin requested review from potiuk, ashb and kaxil as code owners September 5, 2022 09:32

potiuk approved these changes Sep 5, 2022

View reviewed changes

uranusjr reviewed Sep 6, 2022

View reviewed changes

tests/www/views/test_views_log.py Outdated Show resolved Hide resolved

uranusjr approved these changes Sep 7, 2022

View reviewed changes

potiuk approved these changes Sep 7, 2022

View reviewed changes

potiuk mentioned this pull request Sep 9, 2022

Suggesting to add documentation on how to run Airflow on Windows using WSL #26274

Open

2 tasks

kokorin and others added 8 commits September 19, 2022 00:04

Fix: git clone on Windows

0f47fd9

Git clone on Windows: proof of concept

ba69687

Git clone on Windows: rework tests: create expected log files

bb91fd4

Git clone on Windows: fix test_get_logs_for_changed_filename_format_db

f7adf98

Git clone on Windows: delete FileTaskHandlerWinCompat

e96fd1a

Git clone on Windows: fix code formatting

5bf3357

Git clone on Windows: more descriptive comment

75bb5e9

Git clone on Windows: iterdir

a39c7a1

potiuk approved these changes Sep 19, 2022

View reviewed changes

potiuk merged commit 9aa589e into apache:main Sep 19, 2022

kokorin deleted the fix/git-clone-on-windows branch September 19, 2022 14:23

ephraimbuddy pushed a commit that referenced this pull request Nov 10, 2022

Fix: git clone on Windows (#26105)

83837d2

(cherry picked from commit 9aa589e)

ephraimbuddy pushed a commit that referenced this pull request Nov 10, 2022

Fix: git clone on Windows (#26105)

e32ddcd

(cherry picked from commit 9aa589e)

ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Nov 10, 2022

ephraimbuddy added this to the Airflow 2.4.3 milestone Nov 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: git clone on Windows #26105

Fix: git clone on Windows #26105

kokorin commented Sep 1, 2022 •

edited

Loading

boring-cyborg bot commented Sep 1, 2022

potiuk commented Sep 1, 2022

kokorin commented Sep 1, 2022

kokorin commented Sep 1, 2022 •

edited

Loading

uranusjr commented Sep 2, 2022

kokorin commented Sep 2, 2022

kokorin commented Sep 2, 2022

uranusjr commented Sep 2, 2022

potiuk commented Sep 2, 2022

potiuk left a comment

potiuk commented Sep 5, 2022

potiuk commented Sep 5, 2022

kokorin commented Sep 6, 2022

uranusjr Sep 6, 2022 •

edited

Loading

kokorin Sep 6, 2022

uranusjr Sep 6, 2022

kokorin Sep 6, 2022

potiuk commented Sep 6, 2022

kokorin commented Sep 6, 2022

potiuk commented Sep 6, 2022 •

edited

Loading

kokorin commented Sep 7, 2022

uranusjr commented Sep 7, 2022

potiuk commented Sep 7, 2022

kokorin commented Sep 7, 2022

potiuk commented Sep 9, 2022

potiuk commented Sep 9, 2022

kokorin commented Sep 13, 2022

potiuk commented Sep 18, 2022

kokorin commented Sep 19, 2022

boring-cyborg bot commented Sep 19, 2022

potiuk commented Sep 19, 2022

		# delete logs to minimize tests interference
		shutil.rmtree(log_path)

Fix: git clone on Windows #26105

Fix: git clone on Windows #26105

Conversation

kokorin commented Sep 1, 2022 • edited Loading

boring-cyborg bot commented Sep 1, 2022

potiuk commented Sep 1, 2022

kokorin commented Sep 1, 2022

kokorin commented Sep 1, 2022 • edited Loading

uranusjr commented Sep 2, 2022

kokorin commented Sep 2, 2022

kokorin commented Sep 2, 2022

uranusjr commented Sep 2, 2022

potiuk commented Sep 2, 2022

potiuk left a comment

Choose a reason for hiding this comment

potiuk commented Sep 5, 2022

potiuk commented Sep 5, 2022

kokorin commented Sep 6, 2022

uranusjr Sep 6, 2022 • edited Loading

Choose a reason for hiding this comment

kokorin Sep 6, 2022

Choose a reason for hiding this comment

uranusjr Sep 6, 2022

Choose a reason for hiding this comment

kokorin Sep 6, 2022

Choose a reason for hiding this comment

potiuk commented Sep 6, 2022

kokorin commented Sep 6, 2022

potiuk commented Sep 6, 2022 • edited Loading

kokorin commented Sep 7, 2022

uranusjr commented Sep 7, 2022

potiuk commented Sep 7, 2022

kokorin commented Sep 7, 2022

potiuk commented Sep 9, 2022

potiuk commented Sep 9, 2022

kokorin commented Sep 13, 2022

potiuk commented Sep 18, 2022

kokorin commented Sep 19, 2022

boring-cyborg bot commented Sep 19, 2022

potiuk commented Sep 19, 2022

kokorin commented Sep 1, 2022 •

edited

Loading

kokorin commented Sep 1, 2022 •

edited

Loading

uranusjr Sep 6, 2022 •

edited

Loading

potiuk commented Sep 6, 2022 •

edited

Loading