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 2020/10/14 21:22:47 UTC
[GitHub] [airflow] potiuk opened a new pull request #11535: Fix random kills during pre-commit image building
potiuk opened a new pull request #11535:
URL: https://github.com/apache/airflow/pull/11535
Seems like the trap with several steps and || true does not really
work the way I wanted and when kill is run but the process is
already gone, we had error in the script.
Looks like this approach with sub-process kill will do it.
<!--
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 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/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+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 [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #11535: Fix random kills during pre-commit image building
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #11535:
URL: https://github.com/apache/airflow/pull/11535#issuecomment-711151635
@turbaszek :)? You started to use Breeze more recently I belive ;)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #11535: Fix random kills during pre-commit image building
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #11535:
URL: https://github.com/apache/airflow/pull/11535#issuecomment-711388471
Hey @kaxil - look at my explaination. It is correct and exactly as intended. I do want the SPIN_PID to be evaluated when the trap is DEFINED and not when it is executed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #11535: Fix random kills during pre-commit image building
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #11535:
URL: https://github.com/apache/airflow/pull/11535
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #11535: Fix random kills during pre-commit image building
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #11535:
URL: https://github.com/apache/airflow/pull/11535#discussion_r507187187
##########
File path: scripts/ci/libraries/_build_images.sh
##########
@@ -519,8 +519,8 @@ function build_images::build_ci_image() {
" >"${DETECTED_TERMINAL}"
spinner::spin "${OUTPUT_LOG}" &
SPIN_PID=$!
- # shellcheck disable=SC2064
- traps::add_trap "kill ${SPIN_PID} || true" INT TERM HUP EXIT
+ # shellcheck disable=SC2064,SC2016
+ traps::add_trap '$(kill '${SPIN_PID}' || true)' EXIT HUP INT TERM
Review comment:
Example with single quote:
```
❯ a="x"
❯ echo '$a'
$a
❯ echo '${a}'
${a}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #11535: Fix random kills during pre-commit image building
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #11535:
URL: https://github.com/apache/airflow/pull/11535#discussion_r507187187
##########
File path: scripts/ci/libraries/_build_images.sh
##########
@@ -519,8 +519,8 @@ function build_images::build_ci_image() {
" >"${DETECTED_TERMINAL}"
spinner::spin "${OUTPUT_LOG}" &
SPIN_PID=$!
- # shellcheck disable=SC2064
- traps::add_trap "kill ${SPIN_PID} || true" INT TERM HUP EXIT
+ # shellcheck disable=SC2064,SC2016
+ traps::add_trap '$(kill '${SPIN_PID}' || true)' EXIT HUP INT TERM
Review comment:
Example with single quote:
```
❯ a="x"
❯ echo '$a'
$a
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #11535: Fix random kills during pre-commit image building
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #11535:
URL: https://github.com/apache/airflow/pull/11535#discussion_r507187117
##########
File path: scripts/ci/libraries/_build_images.sh
##########
@@ -519,8 +519,8 @@ function build_images::build_ci_image() {
" >"${DETECTED_TERMINAL}"
spinner::spin "${OUTPUT_LOG}" &
SPIN_PID=$!
- # shellcheck disable=SC2064
- traps::add_trap "kill ${SPIN_PID} || true" INT TERM HUP EXIT
+ # shellcheck disable=SC2064,SC2016
+ traps::add_trap '$(kill '${SPIN_PID}' || true)' EXIT HUP INT TERM
Review comment:
```suggestion
traps::add_trap '$(kill "${SPIN_PID}" || true)' EXIT HUP INT TERM
```
Does it not need `"` for expansion?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #11535: Fix random kills during pre-commit image building
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #11535:
URL: https://github.com/apache/airflow/pull/11535#discussion_r507187187
##########
File path: scripts/ci/libraries/_build_images.sh
##########
@@ -519,8 +519,8 @@ function build_images::build_ci_image() {
" >"${DETECTED_TERMINAL}"
spinner::spin "${OUTPUT_LOG}" &
SPIN_PID=$!
- # shellcheck disable=SC2064
- traps::add_trap "kill ${SPIN_PID} || true" INT TERM HUP EXIT
+ # shellcheck disable=SC2064,SC2016
+ traps::add_trap '$(kill '${SPIN_PID}' || true)' EXIT HUP INT TERM
Review comment:
Example with single quote:
```
❯ a="x"
❯ echo '$a'
$a
❯ echo '${a}'
${a}
❯ echo "${a}"
x
❯ e
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #11535: Fix random kills during pre-commit image building
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #11535:
URL: https://github.com/apache/airflow/pull/11535#discussion_r507200556
##########
File path: scripts/ci/libraries/_build_images.sh
##########
@@ -519,8 +519,8 @@ function build_images::build_ci_image() {
" >"${DETECTED_TERMINAL}"
spinner::spin "${OUTPUT_LOG}" &
SPIN_PID=$!
- # shellcheck disable=SC2064
- traps::add_trap "kill ${SPIN_PID} || true" INT TERM HUP EXIT
+ # shellcheck disable=SC2064,SC2016
+ traps::add_trap '$(kill '${SPIN_PID}' || true)' EXIT HUP INT TERM
Review comment:
Nope. It is entirely correct. Look where the scope of ' starts/ends.
I want the SPIN_PID to be resolved at the time when the add_trap command line parameter gets evaluated. The solution you proposed will make it evaluated at the moment trap will be executed.
```
SPIN_PID=10
traps::add_trap '$(kill '${SPIN_PID}' || true)' EXIT HUP INT TERM
```
will be evaluated to
```
traps::add_trap '$(kill 10 || true)' EXIT HUP INT TERM
```
Simply the ${SPIN_PID} is between the two '-delimite striings
And this is exactly what I want.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org