You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "amoghrajesh (via GitHub)" <gi...@apache.org> on 2023/07/06 07:59:27 UTC
[GitHub] [airflow] amoghrajesh opened a new pull request, #32394: Migrating helm chart tests to test-charts to prevent test DB init
amoghrajesh opened a new pull request, #32394:
URL: https://github.com/apache/airflow/pull/32394
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
Seems that pytest will walk up looking for other conftest.py files beyond the tests/charts repo due to which the helm chart tests require DB init to be done which should not be the case. The tests inside `tests` finds the nearest `_clear_db` fixture and continues to use it. We should not use that conftest fixture for helm chart tests.
So likely then https://github.com/apache/airflow/pull/28631 had side-effect of trying to clear db where it was not initialized
This PR moves the helm charts tests outside `tests/charts` to `test-charts`. Tested using the steps:
1. Created a new env
2. Ran the command
`pip install -e ."[devel]" --constraint https://raw.githubusercontent.com/apache/airflow/constraints-main/constraints-source-providers-3.8.txt `
4. Ran the tests in test-charts and we are good :+1:! Did not have to init any DB at all
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code 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] amoghrajesh commented on pull request #32394: Migrating helm chart tests to test-charts to prevent test DB init
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #32394:
URL: https://github.com/apache/airflow/pull/32394#issuecomment-1623398736
Woops. Looks like the IDE didnt catch it all. Made them manually
--
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 #32394: Migrating helm chart tests to helm_tests to prevent test DB init
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32394:
URL: https://github.com/apache/airflow/pull/32394#issuecomment-1623547681
Ah.. Looking at the generated image, there is one more problem:
https://github.com/apache/airflow/blob/main/dev/breeze/src/airflow_breeze/global_constants.py#L123
This is why you had this "static check" failures. The image generation should not be needed, but there is one more place where "tests" / "charts" was used and now the list of allowed packages is wrong.
--
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 #32394: Migrating helm chart tests to helm_tests to prevent test DB init
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32394:
URL: https://github.com/apache/airflow/pull/32394#issuecomment-1623576272
> Also I think the pycache link you pasted is wrong
Should be pointing to :
```
@lru_cache(maxsize=None)
def all_helm_test_packages() -> list[str]:
return sorted(
[
candidate.name
for candidate in (AIRFLOW_SOURCES_ROOT / "tests" / "charts").iterdir()
if candidate.is_dir()
]
)
```
--
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 #32394: Migrating helm chart tests to helm_tests to prevent test DB init
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32394:
URL: https://github.com/apache/airflow/pull/32394#issuecomment-1623664356
For some reason, in your environment there is `__pycache__` folder in the "helm-tests" folder - just filter it out
--
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] amoghrajesh commented on pull request #32394: Migrating helm chart tests to helm_tests to prevent test DB init
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #32394:
URL: https://github.com/apache/airflow/pull/32394#issuecomment-1623651711
The CI is still complaining about failing static checks. Not able to repro it in my local. Also i think the failing UTs are unrelated here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on pull request #32394: Migrating helm chart tests to test-charts to prevent test DB init
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #32394:
URL: https://github.com/apache/airflow/pull/32394#issuecomment-1623319191
All these points are valid. Making the changes to accommodate 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.
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 #32394: Migrating helm chart tests to helm_tests to prevent test DB init
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32394:
URL: https://github.com/apache/airflow/pull/32394#issuecomment-1623486727
Looks good - only static checks need to be fixed
--
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 #32394: Migrating helm chart tests to helm_tests to prevent test DB init
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32394:
URL: https://github.com/apache/airflow/pull/32394#issuecomment-1623776198
BTW. And yeah, those are all (except __pycache__ problems I kind of expected to see in this PR, so this is not really strange or unexpected :)
--
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 #32394: Migrating helm chart tests to helm_tests to prevent test DB init
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #32394:
URL: https://github.com/apache/airflow/pull/32394
--
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 #32394: Migrating helm chart tests to helm_tests to prevent test DB init
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32394:
URL: https://github.com/apache/airflow/pull/32394#issuecomment-1623519317
> Not sure what the failure is. doesn't look related to the changes in this PR
Look at the output of it. It says exactly what it is - and prints you the command tat you should run to fix it. It's about changed `breeze` commmands that need regeneration of embedded Breeze images.
--
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 #32394: Migrating helm chart tests to helm_tests to prevent test DB init
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32394:
URL: https://github.com/apache/airflow/pull/32394#issuecomment-1623772833
Now.... Coming back to unit tests that faik .. There seems to be coming from that chang. For example the first one determines the folder where chart is - this way:
```
chart_dir = Path(__file__).parents[3] / "chart"
```
And after the tests were moved from the "tests/charts" to "helm-tests" - this is no longer working - because instead of looking for chart in "/opt/airflow/charts", it looks in "/opt/charts".
So I think you need to also take a look at those tests that are failling and fix similar cases.
--
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 #32394: Migrating helm chart tests to helm_tests to prevent test DB init
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32394:
URL: https://github.com/apache/airflow/pull/32394#issuecomment-1623587282
Ah. And now "Unit tests Helm" are running just fine in Your PR, so this problem seems to be fixed.
--
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 #32394: Migrating helm chart tests to helm_tests to prevent test DB init
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32394:
URL: https://github.com/apache/airflow/pull/32394#issuecomment-1623956912
🤞
--
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 a diff in pull request #32394: Migrating helm chart tests to helm_tests to prevent test DB init
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32394:
URL: https://github.com/apache/airflow/pull/32394#discussion_r1254846980
##########
helm_tests/airflow_aux/test_pod_template_file.py:
##########
@@ -29,7 +29,7 @@
@pytest.fixture(scope="class", autouse=True)
def isolate_chart(request):
- chart_dir = Path(__file__).parents[3] / "chart"
+ chart_dir = Path(__file__).parents[3] / "airflow" / "chart"
Review Comment:
```suggestion
chart_dir = Path(__file__).parents[2] / "chart"
```
This is how it should be. Having "airflow" hard-coded, you assume that that Airflow has been checked out as "airflow" directory, which does not necessarily needs to be true. This would cause subtle problems that could be very difficult to fix when you will be running the tests in loca environment.
You can easily checkout airflow with a different "folder" name or even create a work-tree if you work on different branches in the same directory where you keep several different versions of airflow checked out.
For example I have:
```
code
| - airflow -> here I mostly work on main
| - airflow-copy -> here I mostly work on v2-6-test branch or similar
```
The airflow-copy is a worktree of the "airflow" repoository - which means that I have one git repository but I can have different branches of the same repo checked out in different folders.
If you do `parents[3] / "airflow" / "chart"`, what will happen is that when I run tests in "airflow-copy", it will copy chart from the "airflow" folder, which might be an old tag or branch. That would be extremely difficult test failure to trace in those cases.
--
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 #32394: Migrating helm chart tests to helm_tests to prevent test DB init
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32394:
URL: https://github.com/apache/airflow/pull/32394#issuecomment-1623548415
You can switch to "swipe" view for the image generated and you will see what happened.
--
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 #32394: Migrating helm chart tests to test-charts to prevent test DB init
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32394:
URL: https://github.com/apache/airflow/pull/32394#issuecomment-1623386516
> All these points are valid. Making the changes to accommodate them
still some "test-charts" in 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 #32394: Migrating helm chart tests to helm_tests to prevent test DB init
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32394:
URL: https://github.com/apache/airflow/pull/32394#issuecomment-1623785113
> Oh, thanks for explaining so patiently. I could not see the folder because it was a hidden one! Silly me
Not a problem, I think there is a value in doing it, as you will learn quite a bit more on how things work in Airflow CI/Breeze/tests etc. - I know well things that are obvious to me as I worked on it for quite some time, aren't necessary obvious to others. I think the important thing is that YOU do not loose the patience, and will go through all that with open approach accepting that there are completely new things coming up that you have to learn about and where this "simple" PR will take quite some time to complete, and that things will be looking wrong at times. There are quite a few moving pieces here that make the whole system works - the fact that you have breeze where you can run it all manually, local venv, but also CI that uses breeze and reproducible enviroment make it much more complex (but not complicated) I think. Complex in the sense that there are multiple pieces of puzzle that work together and each has a role, but every single one is simple (once you know the ro
le, and know that it is there at all).
--
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] amoghrajesh commented on pull request #32394: Migrating helm chart tests to helm_tests to prevent test DB init
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #32394:
URL: https://github.com/apache/airflow/pull/32394#issuecomment-1623938708
Hopefully we should get a green CI now. I have run the tests in the whole module. All looks good 👍🏽
Will also rebase to main so that any unknowns don't show up.
--
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 a diff in pull request #32394: Migrating helm chart tests to helm_tests to prevent test DB init
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32394:
URL: https://github.com/apache/airflow/pull/32394#discussion_r1254848186
##########
helm_tests/airflow_aux/test_pod_template_file.py:
##########
@@ -29,7 +29,7 @@
@pytest.fixture(scope="class", autouse=True)
def isolate_chart(request):
- chart_dir = Path(__file__).parents[3] / "chart"
+ chart_dir = Path(__file__).parents[3] / "airflow" / "chart"
Review Comment:
(Also in other places)
--
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 #32394: Migrating helm chart tests to helm_tests to prevent test DB init
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32394:
URL: https://github.com/apache/airflow/pull/32394#issuecomment-1623996828
1 test left it seems :)
--
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] amoghrajesh commented on a diff in pull request #32394: Migrating helm chart tests to helm_tests to prevent test DB init
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #32394:
URL: https://github.com/apache/airflow/pull/32394#discussion_r1255266415
##########
helm_tests/airflow_aux/test_pod_template_file.py:
##########
@@ -29,7 +29,7 @@
@pytest.fixture(scope="class", autouse=True)
def isolate_chart(request):
- chart_dir = Path(__file__).parents[3] / "chart"
+ chart_dir = Path(__file__).parents[3] / "airflow" / "chart"
Review Comment:
Great point. Thanks for the explanation. I will make the 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.
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 #32394: Migrating helm chart tests to helm_tests to prevent test DB init
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32394:
URL: https://github.com/apache/airflow/pull/32394#issuecomment-1625485435
:tada: :tada:
--
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] amoghrajesh commented on pull request #32394: Migrating helm chart tests to helm_tests to prevent test DB init
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #32394:
URL: https://github.com/apache/airflow/pull/32394#issuecomment-1623935237
Thanks for the detailed mentoring, @potiuk
You have been very helpful and now I know quite some areas confidently in airflow :)
--
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 #32394: Migrating helm chart tests to helm_tests to prevent test DB init
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32394:
URL: https://github.com/apache/airflow/pull/32394#issuecomment-1623513072
There is one unnerving thing here as well - for some reason (and I have no idea why yet) I cannot see the "Unit tests: Helm" running in this PR. I will take a look after you fix all the static checks (and please rebase to latest main as well).
--
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] amoghrajesh commented on pull request #32394: Migrating helm chart tests to helm_tests to prevent test DB init
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #32394:
URL: https://github.com/apache/airflow/pull/32394#issuecomment-1623517351
Let me rebase and let the CI run once. I am not sure what is failing in the static checks. It is not so clear from logs
--
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 #32394: Migrating helm chart tests to helm_tests to prevent test DB init
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32394:
URL: https://github.com/apache/airflow/pull/32394#issuecomment-1623523268
<img width="803" alt="Screenshot 2023-07-06 at 13 33 56" src="https://github.com/apache/airflow/assets/595491/44ed4e4b-514e-4c02-bc1b-7856f83f94e8">
--
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] amoghrajesh commented on pull request #32394: Migrating helm chart tests to helm_tests to prevent test DB init
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #32394:
URL: https://github.com/apache/airflow/pull/32394#issuecomment-1623566143
Also I think the pycache link you pasted is wrong
--
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 #32394: Migrating helm chart tests to helm_tests to prevent test DB init
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32394:
URL: https://github.com/apache/airflow/pull/32394#issuecomment-1623661851
They are failing because of the `__pycache__`. Look at `--helm-test-package` help output:
--
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] amoghrajesh commented on pull request #32394: Migrating helm chart tests to helm_tests to prevent test DB init
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #32394:
URL: https://github.com/apache/airflow/pull/32394#issuecomment-1624002631
@potiuk I think we are done with it now. Hopefully this time it will :)
--
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] amoghrajesh commented on pull request #32394: Migrating helm chart tests to helm_tests to prevent test DB init
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #32394:
URL: https://github.com/apache/airflow/pull/32394#issuecomment-1623511165
Yeah I have made the changes there right: https://github.com/apache/airflow/pull/32394/files#diff-c6f469502ccc758f597b999e8ab5aebeba2ae82fb6dc8166e7d8a25969bcf3faR129
--
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 #32394: Migrating helm chart tests to helm_tests to prevent test DB init
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32394:
URL: https://github.com/apache/airflow/pull/32394#issuecomment-1623551117
BTW. It would also be great to filter out the `__pycache__` folder from https://github.com/apache/airflow/blob/main/dev/breeze/src/airflow_breeze/global_constants.py#L123 - for some reason you had it in your environment, and we should filter it out then.
--
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] amoghrajesh commented on pull request #32394: Migrating helm chart tests to helm_tests to prevent test DB init
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #32394:
URL: https://github.com/apache/airflow/pull/32394#issuecomment-1623565830
@potiuk what is "swipe" view and how do I see it?
--
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] amoghrajesh commented on pull request #32394: Migrating helm chart tests to helm_tests to prevent test DB init
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #32394:
URL: https://github.com/apache/airflow/pull/32394#issuecomment-1624167218
Clean build finally, @potiuk 🙂
--
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] amoghrajesh commented on pull request #32394: Migrating helm chart tests to helm_tests to prevent test DB init
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #32394:
URL: https://github.com/apache/airflow/pull/32394#issuecomment-1623679226
Oh, thanks for explaining so patiently. I could not see the folder because it was a hidden one! Silly me
--
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] amoghrajesh commented on pull request #32394: Migrating helm chart tests to helm_tests to prevent test DB init
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #32394:
URL: https://github.com/apache/airflow/pull/32394#issuecomment-1623501304
Not sure what the failure is. doesn't look related to the changes in this PR
--
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] amoghrajesh commented on pull request #32394: Migrating helm chart tests to helm_tests to prevent test DB init
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #32394:
URL: https://github.com/apache/airflow/pull/32394#issuecomment-1623522326
Thought that was a false alarm. Checking it
--
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] amoghrajesh commented on pull request #32394: Migrating helm chart tests to helm_tests to prevent test DB init
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #32394:
URL: https://github.com/apache/airflow/pull/32394#issuecomment-1623643964
> BTW. It would also be great to filter out the `__pycache__` folder from https://github.com/apache/airflow/blob/main/dev/breeze/src/airflow_breeze/global_constants.py#L123 - for some reason you had it in your environment, and we should filter it out then.
@potiuk I am sorry but I dont understand this point. Where do you see pycache being added?
--
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 a diff in pull request #32394: Migrating helm chart tests to helm_tests to prevent test DB init
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32394:
URL: https://github.com/apache/airflow/pull/32394#discussion_r1254846980
##########
helm_tests/airflow_aux/test_pod_template_file.py:
##########
@@ -29,7 +29,7 @@
@pytest.fixture(scope="class", autouse=True)
def isolate_chart(request):
- chart_dir = Path(__file__).parents[3] / "chart"
+ chart_dir = Path(__file__).parents[3] / "airflow" / "chart"
Review Comment:
```suggestion
chart_dir = Path(__file__).parents[2] / "chart"
```
This is how it should be. Having "airflow" hard-coded, you assume that that Airflow has been checked out as "airflow" directory, which does not necessarily needs to be true. This would cause subtle problems that could be very difficult to fix when you will be running the tests in loca environment.
You can easily checkout airflow with a different "folder" name or even create a work-tree if you work on different branches in the same directory where you keep several different versions of airflow checked out.
For example I have:
```
code
| - airflow -> here I mostly work on main
| - airflow-copy -> here I mostly work on v2-6-test branch or similar
```
The airflow-copy is a worktree of the "airflow" repoository - which means that I have one git repository but I can have different branches of the same repo checked out in different folders.
If you do `parents[3] / "airflow" / "chart"`, what will happen is that when I run tests in "airflow-copy", it will copy chart from the "airflow" folder, which might be an old tag or branch. That would be extremely difficult bug to trace in those cases.
--
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 a diff in pull request #32394: Migrating helm chart tests to helm_tests to prevent test DB init
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32394:
URL: https://github.com/apache/airflow/pull/32394#discussion_r1254846980
##########
helm_tests/airflow_aux/test_pod_template_file.py:
##########
@@ -29,7 +29,7 @@
@pytest.fixture(scope="class", autouse=True)
def isolate_chart(request):
- chart_dir = Path(__file__).parents[3] / "chart"
+ chart_dir = Path(__file__).parents[3] / "airflow" / "chart"
Review Comment:
```suggestion
chart_dir = Path(__file__).parents[2] / "chart"
```
This is how it should be. Having "airflow" hard-coded, you assume that that Airflow has been checked out as "airflow" directory, which does not necessarily needs to be true. This would cause subtle problems that could be very difficult to fix when you will be running the tests in loca environment.
You can easily checkout airflow with a different "folder" name or even create a work-tree if you work on different branches in the same directory where you keep several different versions of airflow checked out.
For example I have:
```
code
| - airflow -> here I mostly work on main
| - airflow-copy -> here I mostly work on v2-6-test branch or similar
```
The airflow-copy is a worktree of the "airflow" repoository - which means that I have one git repository but I can have different branches of the same repo checked out in different folders.
If you do `parents[3] / "airflow" / "chart"`, what will happen is that when I run tests in "airflow-copy", it will copy chart from the "airflow-copy", which might be an old tag or branch. That would be extremely difficult bug to trace in those cases.
--
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 #32394: Migrating helm chart tests to helm_tests to prevent test DB init
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32394:
URL: https://github.com/apache/airflow/pull/32394#issuecomment-1623579628
> @potiuk what is "swipe" view and how do I see it?
Look at the image change in the PR:
https://github.com/apache/airflow/pull/32394/files#diff-fecf419fa1f28e72b014a8962af89c6376d0e17a209ecf2544e933b35515b4ba
<img width="1678" alt="Screenshot 2023-07-06 at 14 15 16" src="https://github.com/apache/airflow/assets/595491/2bf0ec85-b036-4ea6-84cd-d3ec142a2d96">
You will see "deleted" and "added" image. You can choose "swipe" and then you can swipe the handle at the bottom and see the difference clearly as you swipe.
Those images are abit small, but when you zoom in the browser window it's great way to see what's changed in an image in PR.
--
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] amoghrajesh commented on pull request #32394: Migrating helm chart tests to helm_tests to prevent test DB init
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #32394:
URL: https://github.com/apache/airflow/pull/32394#issuecomment-1625179595
@potiuk has any new hook been added to check for re2 library over re? The static checks fail with the error..
```
Use re2 over re....................................................................Failed
- hook id: check-usage-of-re2-over-re
- exit code: 1
helm_tests/airflow_aux/test_pod_template_file.py:19:import re
helm_tests/other/test_redis.py:19:import re
``
--
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