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/03/16 21:55:49 UTC
[GitHub] [airflow] edithturn opened a new pull request #22327: Rewrite Selective Check in Python
edithturn opened a new pull request #22327:
URL: https://github.com/apache/airflow/pull/22327
closes #19971
--
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 #22327: Rewrite Selective Check in Python
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #22327:
URL: https://github.com/apache/airflow/pull/22327#issuecomment-1073209858
> Direct Push (PR is merged) doesn't generate a commit hash (SHA)
It also generates commit hash, but we do not care about it. For Direct Push/Merge we always want to run all posible tests so in this case selective check turns into "run everything possible".
> Merge Commit generates a commit hash
Yep. We have it in all cases (because we are using it to assign the "tag" to images we build). But in case of Pull Request Github prepares a "merge commit" containing only changes coming from the PR, and the commit HASH we have is this particular commit.
> CHANGED_FILES=$(git diff-tree --no-commit-id --name-only -r "${INCOMING_COMMIT_SHA}^" "${INCOMING_COMMIT_SHA} || true)
This is just a way how you can get the list of files when you get an incoming commit:
1) git diff-tree produces information about difference between two commits
2) --no-commit-id --name-only -> this causes git diff-tree to only show file names that were changed between the two commits
3) -r "${INCOMING_COMMIT_SHA}^" -> this is the parent (^) of the incoming merge commit
4) "${INCOMING_COMMIT_SHA}" -> this is the incoming commit
So what we get at the end is the list of changed files between the "parent of commit" and "commit" that is incoming from the 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] potiuk edited a comment on pull request #22327: Rewrite Selective Check in Python
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #22327:
URL: https://github.com/apache/airflow/pull/22327#issuecomment-1073211644
>I saw also that this symbol "^" in bash is to keep lower case the first letter of a string (In python I am having the same issue "ambiguous argument")
The `^` is not a bash construct. It's GIT's "commit parent". It is so called "commit-ish" reference - one of the many ways you can refer to a commit in git.
For example `HEAD^` means direct (first) parent of the commit, `HEAD^^` or `HEAD~2` is (following first parents grand-parent and `HEAD^2` means second parent (if commit is a merge commit and has more than one parent).
All the "commit-ish" references can be found here: https://mirrors.edge.kernel.org/pub/software/scm/git/docs/gitrevisions.html#_specifying_revisions
BTW. one of the great things in our "rebase" workflow is that we have no merge commits. Every commit (after pull request is merged) has exactly one parent, so it makes all the reasoning about changes WAY simpler (as you do not have commits with multiple parents, you know exactly which files has changed by making a diff between the commit and it's parent).
--
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] edithturn commented on pull request #22327: Rewrite Selective Check in Python
Posted by GitBox <gi...@apache.org>.
edithturn commented on pull request #22327:
URL: https://github.com/apache/airflow/pull/22327#issuecomment-1072915862
@potiuk I created two different files: scc_get_changed_files.py and selective_ci_checks.py and in ci.yaml I am just calling to selective_ci_checks.py.
I have a doubt about this statement:
I understand this:
1. Direct Push (PR is **merged**) doesn't generate a commit hash (SHA)
2. **Merge** Commit generates a commit hash
Both are "merged" why **Direct Push** doesn't generate a commit SHA?
On the other hand, could you explain to me the selected part of this script?
I know that we are trying to get all the names of the changed files from a commit SHA. I saw also that this symbol "^" in bash is to keep lower case the first letter of a string (In python I am having the same issue "ambiguous argument")
Thanks in advance!
This is a great task, I am getting fun 💃🏼
--
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 #22327: Rewrite Selective Check in Python
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #22327:
URL: https://github.com/apache/airflow/pull/22327#issuecomment-1084261551
> @potiuk I have a doubt about how to send the parameter **"${GITHUB_SHA}"** to the entry point in ci.yaml, I was thinking something like this:` airflow-selective-checks ${GITHUB_SHA}` I also think it should be like "freespace", when we run it we can send parameters like --help and others.
>
> What would be the best here?
add --github-sha as Click parameter with GITHUB_SHA envvar. That should work.
--
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] edithturn edited a comment on pull request #22327: Rewrite Selective Check in Python
Posted by GitBox <gi...@apache.org>.
edithturn edited a comment on pull request #22327:
URL: https://github.com/apache/airflow/pull/22327#issuecomment-1072915862
@potiuk I created two different files: scc_get_changed_files.py and selective_ci_checks.py and in ci.yaml I am just calling to selective_ci_checks.py.
I have a doubt about this statement:
I understand this:
1. Direct Push (PR is **merged**) doesn't generate a commit hash (SHA)
2. **Merge** Commit generates a commit hash
Both are "merged" why **Direct Push** doesn't generate a commit SHA?
On the other hand, could you explain to me the selected part of this script?
I know that we are trying to get all the names of the changed files from a commit SHA. I saw also that this symbol "^" in bash is to keep lower case the first letter of a string (In python I am having the same issue "ambiguous argument")
https://github.com/apache/airflow/blob/0ec56775df66063cab807d886e412ebf88c572bf/scripts/ci/selective_ci_checks.sh#L154-L155
Thanks in advance!
This is a great task, I am getting fun 💃🏼
--
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] edithturn commented on pull request #22327: Rewrite Selective Check in Python
Posted by GitBox <gi...@apache.org>.
edithturn commented on pull request #22327:
URL: https://github.com/apache/airflow/pull/22327#issuecomment-1073353362
Thanks, @potiuk. I am retrieving all the files that are modified in the PR. Let me know now if the statement bellow is in the right way.
Process:
Steps 1, 2, and 3 will be in the Selective Check main script. The other steps will be independent scripts. This is just an example of many other python calls.
**1. First Python Call: Build image**
1.1. Return all changed files
1.2. Validate parameters of the commit like SHA, Kind of PR
1.3 Determinate if the IMAGE should be BUILD
**2. Second Python Call: Python Version to Build**
2.1. Return all changed files
2.2. Validate parameters of the commit like SHA, Kind of PR
2.3 Determinate what version of pythons build
**3. Process the validation of 1.3 and 2.3**
Also I created another step in ci.yaml for selective_ci_checks.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] edithturn edited a comment on pull request #22327: Rewrite Selective Check in Python
Posted by GitBox <gi...@apache.org>.
edithturn edited a comment on pull request #22327:
URL: https://github.com/apache/airflow/pull/22327#issuecomment-1073353362
Thanks, @potiuk. I am retrieving all the files that are modified in the PR. Let me know now if the statement bellow is in the right way.
Process:
Steps 1, 2, and 3 will be in the Selective Check main script. The other steps will be independent scripts. This is just an example of many other python calls.
**1. First Python Call: Build image**
1.1. Return all changed files
1.2. Validate parameters of the commit like SHA, Kind of PR
1.3 Determinate if the IMAGE should be BUILD
**2. Second Python Call: Python Version to Build**
2.1. Return all changed files
2.2. Validate parameters of the commit like SHA, Kind of PR
2.3 Determinate what version of pythons build
**3. Process the validation of 1.3 and 2.3**
Also, I created another step in ci.yaml for selective_ci_checks.py
https://github.com/apache/airflow/runs/5619304320?check_suite_focus=true#step:6:587
--
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 edited a comment on pull request #22327: Rewrite Selective Check in Python
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #22327:
URL: https://github.com/apache/airflow/pull/22327#issuecomment-1073211644
>I saw also that this symbol "^" in bash is to keep lower case the first letter of a string (In python I am having the same issue "ambiguous argument")
The `^` is not a bash construct. It's GIT's "commit parent". It is so called "commit-ish" reference - one of the many ways you can refer to a commit in git.
For example `HEAD^` means direct (first) parent of the commit, `HEAD^^` or `HEAD~2` is (following first parents grand-parent and `HEAD^2` means second parent (if commit is a merge commit and has more than one parent).
All the "commit-ish" references can be found here: https://mirrors.edge.kernel.org/pub/software/scm/git/docs/gitrevisions.html#_specifying_revisions
--
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] edithturn edited a comment on pull request #22327: Rewrite Selective Check in Python
Posted by GitBox <gi...@apache.org>.
edithturn edited a comment on pull request #22327:
URL: https://github.com/apache/airflow/pull/22327#issuecomment-1079476927
@potiuk I understood the idea. Thanks!
I tried to make the first and second points. Created the **entrypoint** for "selective-checks-python" However, for **freespace** we need to install "Setup python" and "python -m pip install --editable ./dev/breeze/" which are installed in another job.
In the case of "selective check python", I added this dependency to make the **entrypoint** work. And I am getting "permission denied" => `[Errno 13] Permission denied: '/usr/lib/python3.8/site-packages'`, Looking for this issue there are 03 possible solutions:
You can see more about these solutions here: https://github.com/googlesamples/assistant-sdk-python/issues/236#issuecomment-748752106
I tried "--user" and "venv", they are not working. build-info is the first job in ci.yaml, for that reason it doesn't have all the inputs necessary to build things. Is this right?
Here is my failed job: https://github.com/apache/airflow/runs/5698251497?check_suite_focus=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] edithturn commented on pull request #22327: Rewrite Selective Check in Python
Posted by GitBox <gi...@apache.org>.
edithturn commented on pull request #22327:
URL: https://github.com/apache/airflow/pull/22327#issuecomment-1082479392
@potiuk I have a doubt about how to send the parameter **"${GITHUB_SHA}"** to the entry point in ci.yaml, I was thinking something like this:` airflow-selective-checks ${GITHUB_SHA}`
I also think it should be like "freespace", when we run it we can send parameters like --help and others.
What would be the best 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] potiuk commented on pull request #22327: Rewrite Selective Check in Python
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #22327:
URL: https://github.com/apache/airflow/pull/22327#issuecomment-1073211644
>I saw also that this symbol "^" in bash is to keep lower case the first letter of a string (In python I am having the same issue "ambiguous argument")
The `^` is not a bash construct. It's GIT's "commit parent". It is so called "commit-ish" reference - one of the many ways you can refer to a commit in git.
For example `HEAD^` means parent of the commit, `HEAD^^` or `HEAD~2` is grand-parent and `HEAD^2` means second parent (if commit is a merge commit and has more than one parent).
All the "commit-ish" references can be found here: https://mirrors.edge.kernel.org/pub/software/scm/git/docs/gitrevisions.html#_specifying_revisions
--
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] edithturn commented on pull request #22327: Rewrite Selective Check in Python
Posted by GitBox <gi...@apache.org>.
edithturn commented on pull request #22327:
URL: https://github.com/apache/airflow/pull/22327#issuecomment-1086550070
@potiuk please ignore that this job is failing, I just wanna know if this is the right way about your last feedback: https://github.com/apache/airflow/pull/22327#issuecomment-1074248212
Please, can see the code of the structure of the files. I restructured the files I am working on.
* Selective check would work like free-space, with python click for matrix strategy.
`airflow-selective-checks -m "matrix_strategy"`
* There would be a python file of all validations and checks.
* The matrix strategy function is calling all these validations before printing python versions (just for now)
--
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 #22327: Rewrite Selective Check in Python
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #22327:
URL: https://github.com/apache/airflow/pull/22327#issuecomment-1074248212
> Thanks, @potiuk. I am retrieving all the files that are modified in the PR. Let me know now if the statement bellow is in the right way.
>
> Process: Steps 1, 2, and 3 will be in the Selective Check main script. The other steps will be independent scripts. This is just an example of many other python calls.
>
> Also, I created another step in ci.yaml for selective_ci_checks.py
>
> https://github.com/apache/airflow/runs/5619304320?check_suite_focus=true#step:6:587
Good start. I think there are lots of good things there :). One thing though I thin we should improve
1) I think all the checks should still be in on file "selective_checks.py". This will make it easier to manage and share code.
2) This `selective_checks.py` should be run through entrypoint (`airflow-selective-checks`) - very similarly to `airlflow-freespace`
3) Then in CI instead of running `./scripts/ci/....` you will run `airlflow-selective-checks` same as freespace :)
4) Now. I think the clue here is to get diffrent commands in this one `airflow-selective-checks`). I can imagine such commands:
* `airflow-selective-checks matrix-strategy`
This one (based on the checks) should either print :
* python-versions:
* `::set-output name=python-versions::[''3.7', '3.8', '3.9', '3.10']` - if we determine that all python versions should be used
* `::set-output name=python-versions::[''3.7']` - if we determine that only this one python version should be used
Plus few more outputs that are related:
* python-versions-list-as-string
* kubernetes-versions
* kubernetes-versions-list-as-string
* all-python-versions
* mysql-versions
* mssql-versions
* default-mssql-version
* .....
* postgres-exclude
* mysql-exclude
And so on - basically all the parameters that determine what should be the matrix of tests executed.
Then the next step will be to build sther commands:
* `airflow-selective-checks image-build-needed`
`::set-output name=image-build::true` or `::set-output name=image-build::true` depending if we determine that images are needed or not
* `airflow-selective-checks doc-build-needed`
similarly but for `docs-build`
* `airflow-selective-checks test-types`
Similarly but for `test-types`
But we can do it gradually. The `matrix-strategy` seems to be good start,
I hope it makes sense :)
--
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] edithturn commented on pull request #22327: Rewrite Selective Check in Python
Posted by GitBox <gi...@apache.org>.
edithturn commented on pull request #22327:
URL: https://github.com/apache/airflow/pull/22327#issuecomment-1079476927
@potiuk I understood the idea. Thanks!
I tried to make the first and second points. Created the **entrypoint** for "selective-checks-python" However, for **freespace** we need to install "Setup python" and "python -m pip install --editable ./dev/breeze/" which are installed in another job.
In the case of "selective check python", I added this dependency to make the **entrypoint** work. And I am getting "permission denied" => `[Errno 13] Permission denied: '/usr/lib/python3.8/site-packages'`, Looking for this issue there are 03 possible solutions:
https://github.com/googlesamples/assistant-sdk-python/issues/236#issuecomment-748752106
I tried "--user" and "venv", they are not working. build-info is the first job in ci.yaml, for that reason it doesn't have all the inputs necessary to build things. Is this right?
https://github.com/apache/airflow/runs/5698251497?check_suite_focus=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] potiuk commented on a change in pull request #22327: Rewrite Selective Check in Python
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #22327:
URL: https://github.com/apache/airflow/pull/22327#discussion_r841268378
##########
File path: dev/breeze/src/airflow_ci/sc_output.py
##########
@@ -0,0 +1,48 @@
+# Licensed to the Apache Software Foundation (ASF) under one
Review comment:
Those files should all go to `selective_checks` package under `ci` folder.
##########
File path: dev/breeze/src/airflow_ci/sc_output.py
##########
@@ -0,0 +1,48 @@
+# Licensed to the Apache Software Foundation (ASF) under one
Review comment:
Those files should all go to `selective_checks` package under `ci` package.
--
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 change in pull request #22327: Rewrite Selective Check in Python
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #22327:
URL: https://github.com/apache/airflow/pull/22327#discussion_r841268591
##########
File path: dev/breeze/src/airflow_ci/selective_ci_utils.py
##########
@@ -0,0 +1,24 @@
+#!/usr/bin/env python3
Review comment:
we do not need "read_only" - it was only to get more sanity for BASH code, but using it in Python is not needed.
--
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