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/06/10 20:53:24 UTC
[GitHub] [airflow] mik-laj opened a new pull request #9219: Fix system tests for DataflowCreateJavaJobOperator
mik-laj opened a new pull request #9219:
URL: https://github.com/apache/airflow/pull/9219
---
Make sure to mark the boxes below before creating PR: [x]
- [X] Description above provides context of the change
- [X] Unit tests coverage for changes (not needed for documentation changes)
- [X] Target Github ISSUE in description if exists
- [X] Commits follow "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)"
- [X] Relevant documentation is updated including usage instructions.
- [X] I will engage committers as explained in [Contribution Workflow Example](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#contribution-workflow-example).
---
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).
Read the [Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines) for more information.
----------------------------------------------------------------
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] mik-laj commented on a change in pull request #9219: Fix system tests for DataflowCreateJavaJobOperator
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #9219:
URL: https://github.com/apache/airflow/pull/9219#discussion_r438476274
##########
File path: scripts/ci/prepare_tool_scripts.sh
##########
@@ -29,19 +29,28 @@ function prepare_tool_script() {
cat >"${TARGET_TOOL_PATH}" <<EOF
#!/usr/bin/env bash
-docker run --rm -it \
- -v "\${HOST_AIRFLOW_SOURCES}/tmp:/tmp" \
- -v "\${HOST_AIRFLOW_SOURCES}/files:/files" \
- -v "\${HOST_AIRFLOW_SOURCES}:/opt/airflow" \
- -v "\${HOST_HOME}/${VOLUME}:/root/${VOLUME}" \
- "${IMAGE}" ${COMMAND} "\$@"
+DOCKER_ARGS=(
+ -v "\${HOST_AIRFLOW_SOURCES}/tmp:/tmp" \\
+ -v "\${HOST_AIRFLOW_SOURCES}/files:/files" \\
+ -v "\${HOST_AIRFLOW_SOURCES}:/opt/airflow" \\
+ -v "\${HOST_HOME}/${VOLUME}:/root/${VOLUME}" \\
+ --interactive \\
+)
+if [ -t 0 ] ; then
+ DOCKER_ARGS+=(
+ --tty \\
+ )
+fi
+
+docker run "\${DOCKER_ARGS[@]}" "${IMAGE}" "\${@}"
+
RES=\$?
if [[ \${HOST_OS} == "Linux" ]]; then
docker run --rm \
- -v "\${HOST_AIRFLOW_SOURCES}/tmp:/tmp" \
- -v "\${HOST_AIRFLOW_SOURCES}/files:/files" \
- -v "\${HOST_HOME}/${VOLUME}:/root/${VOLUME}" \
- "\${AIRFLOW_CI_IMAGE}" bash -c \
+ -v "\${HOST_AIRFLOW_SOURCES}/tmp:/tmp" \\
Review comment:
One-line scripts are very difficult to read.
Before:
```bash
#!/usr/bin/env bash
docker run --rm -it -v "${HOST_AIRFLOW_SOURCES}/tmp:/tmp" -v "${HOST_AIRFLOW_SOURCES}/files:/files" -v "${HOST_AIRFLOW_SOURCES}:/opt/airflow" -v "${HOST_HOME}/.config/gcloud:/root/.config/gcloud" "gcr.io/google.com/cloudsdktool/cloud-sdk:latest" gcloud "$@"
RES=$?
if [[ ${HOST_OS} == "Linux" ]]; then
docker run --rm -v "${HOST_AIRFLOW_SOURCES}/tmp:/tmp" -v "${HOST_AIRFLOW_SOURCES}/files:/files" -v "${HOST_HOME}/.config/gcloud:/root/.config/gcloud" "${AIRFLOW_CI_IMAGE}" bash -c "find '/tmp/' '/files/' '/root/.config/gcloud' -user root -print0 | xargs --null chown '${HOST_USER_ID}.${HOST_GROUP_ID}' --no-dereference" >/dev/null 2>&1
fi
exit ${RES}
```
After:
```bash
#!/usr/bin/env bash
docker run --rm -it \
-v "${HOST_AIRFLOW_SOURCES}/tmp:/tmp" \
-v "${HOST_AIRFLOW_SOURCES}/files:/files" \
-v "${HOST_AIRFLOW_SOURCES}:/opt/airflow" \
-v "${HOST_HOME}/.config/gcloud:/root/.config/gcloud" \
"gcr.io/google.com/cloudsdktool/cloud-sdk:latest" gcloud "$@"
RES=$?
if [[ ${HOST_OS} == "Linux" ]]; then
docker run --rm \
-v "${HOST_AIRFLOW_SOURCES}/tmp:/tmp" \
-v "${HOST_AIRFLOW_SOURCES}/files:/files" \
-v "${HOST_HOME}/.config/gcloud:/root/.config/gcloud" \
"${AIRFLOW_CI_IMAGE}" bash -c \
"find '/tmp/' '/files/' '/root/.config/gcloud' -user root -print0 | xargs --null chown '${HOST_USER_ID}.${HOST_GROUP_ID}' --no-dereference" >/dev/null 2>&1
fi
exit ${RES}
```
----------------------------------------------------------------
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] mik-laj commented on a change in pull request #9219: Fix system tests for DataflowCreateJavaJobOperator
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #9219:
URL: https://github.com/apache/airflow/pull/9219#discussion_r438404330
##########
File path: scripts/ci/prepare_tool_scripts.sh
##########
@@ -59,9 +68,60 @@ GCLOUD_IMAGE="gcr.io/google.com/cloudsdktool/cloud-sdk:latest"
prepare_tool_script "amazon/aws-cli:latest" ".aws" aws
prepare_tool_script "mcr.microsoft.com/azure-cli:latest" ".azure" az az
-prepare_tool_script "${GCLOUD_IMAGE}" ".config/gcloud" bq bq
-prepare_tool_script "${GCLOUD_IMAGE}" ".config/gcloud" gcloud gcloud
-prepare_tool_script "${GCLOUD_IMAGE}" ".config/gcloud" gsutil gsutil
+
+function prepare_gcloud_script() {
+ IMAGE="${1}"
+ TOOL="${2}"
+ COMMAND="${2}"
+ VOLUME=".config/gcloud"
+
+ TARGET_TOOL_PATH="/usr/bin/${TOOL}"
+ TARGET_TOOL_UPDATE_PATH="/usr/bin/${TOOL}-update"
+
+ cat >"${TARGET_TOOL_PATH}" <<EOF
+#!/usr/bin/env bash
+
+DOCKER_ARGS=(
+ --rm \\
+ -v "\${HOST_AIRFLOW_SOURCES}/tmp:/tmp" \\
+ -v "\${HOST_AIRFLOW_SOURCES}/files:/files" \\
+ -v "\${HOST_AIRFLOW_SOURCES}:/opt/airflow" \\
+ -v "\${HOST_HOME}/${VOLUME}:/root/${VOLUME}" \\
+ -e GOOGLE_APPLICATION_CREDENTIALS \\
+ -e CLOUDSDK_CONFIG \\
Review comment:
GOOGLE_APPLICATION_CREDENTIALS and CLOUDSDK_CONFIG are required by ADC.
----------------------------------------------------------------
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 #9219: Fix system tests for DataflowCreateJavaJobOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #9219:
URL: https://github.com/apache/airflow/pull/9219#discussion_r438430716
##########
File path: scripts/ci/docker-compose/local-prod.yml
##########
@@ -35,7 +35,7 @@ services:
- ../../../setup.cfg:/opt/airflow/setup.cfg:cached
- ../../../setup.py:/opt/airflow/setup.py:cached
- ../../../tests:/opt/airflow/tests:cached
- - ../../../tmp:/opt/airflow/tmp:cached
+ - ../../../tmp:/tmp:cached
Review comment:
Interesting. indeed.
----------------------------------------------------------------
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] mik-laj commented on a change in pull request #9219: Fix system tests for DataflowCreateJavaJobOperator
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #9219:
URL: https://github.com/apache/airflow/pull/9219#discussion_r438403264
##########
File path: scripts/ci/docker-compose/local-prod.yml
##########
@@ -35,7 +35,7 @@ services:
- ../../../setup.cfg:/opt/airflow/setup.cfg:cached
- ../../../setup.py:/opt/airflow/setup.py:cached
- ../../../tests:/opt/airflow/tests:cached
- - ../../../tmp:/opt/airflow/tmp:cached
+ - ../../../tmp:/tmp:cached
Review comment:
It was a huge problem for me. It took me a long time to find this error. I did not know why the jar does not start, because for manual testing I had this file saved in /files/. It was very confusing.
----------------------------------------------------------------
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] mik-laj commented on a change in pull request #9219: Fix system tests for DataflowCreateJavaJobOperator
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #9219:
URL: https://github.com/apache/airflow/pull/9219#discussion_r438405110
##########
File path: scripts/ci/prepare_tool_scripts.sh
##########
@@ -72,25 +132,36 @@ function prepare_terraform_script() {
cat >"${TARGET_TOOL_PATH}" <<EOF
#!/usr/bin/env bash
-docker run --rm -it \
- -v "\${HOST_AIRFLOW_SOURCES}/tmp:/tmp" \
- -v "\${HOST_AIRFLOW_SOURCES}/files:/files" \
- -v "\${HOST_AIRFLOW_SOURCES}:/opt/airflow" \
- -v "\${HOST_HOME}/.aws:/root/.aws" \
- -v "\${HOST_HOME}/.azure:/root/.azure" \
- -v "\${HOST_HOME}/.config/gcloud:/root/.config/gcloud" \
- -w /opt/airflow \
- --env-file <(env | grep TF) \
- "${IMAGE}" "\$@"
+
+DOCKER_ARGS=(
+ -v "\${HOST_AIRFLOW_SOURCES}/tmp:/tmp" \\
+ -v "\${HOST_AIRFLOW_SOURCES}/files:/files" \\
+ -v "\${HOST_AIRFLOW_SOURCES}:/opt/airflow" \\
+ -v "\${HOST_HOME}/.aws:/root/.aws" \\
+ -v "\${HOST_HOME}/.azure:/root/.azure" \\
+ -v "\${HOST_HOME}/.config/gcloud:/root/.config/gcloud" \\
+ -w /opt/airflow \\
+ --env-file <(env | grep TF) \\
+ -e GOOGLE_APPLICATION_CREDENTIALS \\
+ -e CLOUDSDK_CONFIG \\
Review comment:
We also need ADC support 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj closed pull request #9219: Fix system tests for DataflowCreateJavaJobOperator
Posted by GitBox <gi...@apache.org>.
mik-laj closed pull request #9219:
URL: https://github.com/apache/airflow/pull/9219
----------------------------------------------------------------
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] mik-laj commented on a change in pull request #9219: Fix system tests for DataflowCreateJavaJobOperator
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #9219:
URL: https://github.com/apache/airflow/pull/9219#discussion_r438404044
##########
File path: scripts/ci/prepare_tool_scripts.sh
##########
@@ -29,19 +29,28 @@ function prepare_tool_script() {
cat >"${TARGET_TOOL_PATH}" <<EOF
#!/usr/bin/env bash
-docker run --rm -it \
- -v "\${HOST_AIRFLOW_SOURCES}/tmp:/tmp" \
- -v "\${HOST_AIRFLOW_SOURCES}/files:/files" \
- -v "\${HOST_AIRFLOW_SOURCES}:/opt/airflow" \
- -v "\${HOST_HOME}/${VOLUME}:/root/${VOLUME}" \
- "${IMAGE}" ${COMMAND} "\$@"
+DOCKER_ARGS=(
+ -v "\${HOST_AIRFLOW_SOURCES}/tmp:/tmp" \\
+ -v "\${HOST_AIRFLOW_SOURCES}/files:/files" \\
+ -v "\${HOST_AIRFLOW_SOURCES}:/opt/airflow" \\
+ -v "\${HOST_HOME}/${VOLUME}:/root/${VOLUME}" \\
+ --interactive \\
+)
+if [ -t 0 ] ; then
+ DOCKER_ARGS+=(
+ --tty \\
+ )
+fi
+
+docker run "\${DOCKER_ARGS[@]}" "${IMAGE}" "\${@}"
+
RES=\$?
if [[ \${HOST_OS} == "Linux" ]]; then
docker run --rm \
- -v "\${HOST_AIRFLOW_SOURCES}/tmp:/tmp" \
- -v "\${HOST_AIRFLOW_SOURCES}/files:/files" \
- -v "\${HOST_HOME}/${VOLUME}:/root/${VOLUME}" \
- "\${AIRFLOW_CI_IMAGE}" bash -c \
+ -v "\${HOST_AIRFLOW_SOURCES}/tmp:/tmp" \\
Review comment:
A single slash in a text string means that the new line is ignored.. We want a new line, so we need a double slash.
----------------------------------------------------------------
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] mik-laj commented on a change in pull request #9219: Fix system tests for DataflowCreateJavaJobOperator
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #9219:
URL: https://github.com/apache/airflow/pull/9219#discussion_r438477694
##########
File path: scripts/ci/prepare_tool_scripts.sh
##########
@@ -59,9 +68,60 @@ GCLOUD_IMAGE="gcr.io/google.com/cloudsdktool/cloud-sdk:latest"
prepare_tool_script "amazon/aws-cli:latest" ".aws" aws
prepare_tool_script "mcr.microsoft.com/azure-cli:latest" ".azure" az az
-prepare_tool_script "${GCLOUD_IMAGE}" ".config/gcloud" bq bq
-prepare_tool_script "${GCLOUD_IMAGE}" ".config/gcloud" gcloud gcloud
-prepare_tool_script "${GCLOUD_IMAGE}" ".config/gcloud" gsutil gsutil
+
+function prepare_gcloud_script() {
+ IMAGE="${1}"
+ TOOL="${2}"
+ COMMAND="${2}"
+ VOLUME=".config/gcloud"
+
+ TARGET_TOOL_PATH="/usr/bin/${TOOL}"
+ TARGET_TOOL_UPDATE_PATH="/usr/bin/${TOOL}-update"
+
+ cat >"${TARGET_TOOL_PATH}" <<EOF
+#!/usr/bin/env bash
+
+DOCKER_ARGS=(
+ --rm \\
+ -v "\${HOST_AIRFLOW_SOURCES}/tmp:/tmp" \\
+ -v "\${HOST_AIRFLOW_SOURCES}/files:/files" \\
+ -v "\${HOST_AIRFLOW_SOURCES}:/opt/airflow" \\
+ -v "\${HOST_HOME}/${VOLUME}:/root/${VOLUME}" \\
+ -e GOOGLE_APPLICATION_CREDENTIALS \\
+ -e CLOUDSDK_CONFIG \\
Review comment:
Agree. I simplified scripts. Now we have one common script instead 7 * 2 + 1 = 15 scripts - https://github.com/apache/airflow/pull/9223/files
We still keep the old behavior.
----------------------------------------------------------------
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] mik-laj commented on a change in pull request #9219: Fix system tests for DataflowCreateJavaJobOperator
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #9219:
URL: https://github.com/apache/airflow/pull/9219#discussion_r438476917
##########
File path: scripts/ci/prepare_tool_scripts.sh
##########
@@ -72,25 +132,36 @@ function prepare_terraform_script() {
cat >"${TARGET_TOOL_PATH}" <<EOF
#!/usr/bin/env bash
-docker run --rm -it \
- -v "\${HOST_AIRFLOW_SOURCES}/tmp:/tmp" \
- -v "\${HOST_AIRFLOW_SOURCES}/files:/files" \
- -v "\${HOST_AIRFLOW_SOURCES}:/opt/airflow" \
- -v "\${HOST_HOME}/.aws:/root/.aws" \
- -v "\${HOST_HOME}/.azure:/root/.azure" \
- -v "\${HOST_HOME}/.config/gcloud:/root/.config/gcloud" \
- -w /opt/airflow \
- --env-file <(env | grep TF) \
- "${IMAGE}" "\$@"
+
+DOCKER_ARGS=(
+ -v "\${HOST_AIRFLOW_SOURCES}/tmp:/tmp" \\
+ -v "\${HOST_AIRFLOW_SOURCES}/files:/files" \\
+ -v "\${HOST_AIRFLOW_SOURCES}:/opt/airflow" \\
+ -v "\${HOST_HOME}/.aws:/root/.aws" \\
+ -v "\${HOST_HOME}/.azure:/root/.azure" \\
+ -v "\${HOST_HOME}/.config/gcloud:/root/.config/gcloud" \\
+ -w /opt/airflow \\
+ --env-file <(env | grep TF) \\
+ -e GOOGLE_APPLICATION_CREDENTIALS \\
+ -e CLOUDSDK_CONFIG \\
Review comment:
Would you like each tool to have access to the credentials of other tools? I thought that it is important to ensure that the container has access to the credentials it needs.
----------------------------------------------------------------
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 #9219: Fix system tests for DataflowCreateJavaJobOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #9219:
URL: https://github.com/apache/airflow/pull/9219#discussion_r438430965
##########
File path: scripts/ci/docker-compose/local.yml
##########
@@ -56,7 +56,7 @@ services:
- ../../../setup.py:/opt/airflow/setup.py:cached
- ../../../tests:/opt/airflow/tests:cached
- ../../../kubernetes_tests:/opt/airflow/kubernetes_tests:cached
- - ../../../tmp:/opt/airflow/tmp:cached
+ - ../../../tmp:/tmp:cached
Review comment:
Note that you need to change it in _local_mounts.sh as it is auto-generated (see comment one line below)
----------------------------------------------------------------
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] mik-laj commented on a change in pull request #9219: Fix system tests for DataflowCreateJavaJobOperator
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #9219:
URL: https://github.com/apache/airflow/pull/9219#discussion_r438403264
##########
File path: scripts/ci/docker-compose/local-prod.yml
##########
@@ -35,7 +35,7 @@ services:
- ../../../setup.cfg:/opt/airflow/setup.cfg:cached
- ../../../setup.py:/opt/airflow/setup.py:cached
- ../../../tests:/opt/airflow/tests:cached
- - ../../../tmp:/opt/airflow/tmp:cached
+ - ../../../tmp:/tmp:cached
Review comment:
It was a huge problem for me. It took me a long time to find this bug. I did not know why the jar does not start, because for manual testing I had this file saved in /files/. It was very confusing.
----------------------------------------------------------------
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 #9219: Fix system tests for DataflowCreateJavaJobOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #9219:
URL: https://github.com/apache/airflow/pull/9219#discussion_r438435306
##########
File path: scripts/ci/prepare_tool_scripts.sh
##########
@@ -72,25 +132,36 @@ function prepare_terraform_script() {
cat >"${TARGET_TOOL_PATH}" <<EOF
#!/usr/bin/env bash
-docker run --rm -it \
- -v "\${HOST_AIRFLOW_SOURCES}/tmp:/tmp" \
- -v "\${HOST_AIRFLOW_SOURCES}/files:/files" \
- -v "\${HOST_AIRFLOW_SOURCES}:/opt/airflow" \
- -v "\${HOST_HOME}/.aws:/root/.aws" \
- -v "\${HOST_HOME}/.azure:/root/.azure" \
- -v "\${HOST_HOME}/.config/gcloud:/root/.config/gcloud" \
- -w /opt/airflow \
- --env-file <(env | grep TF) \
- "${IMAGE}" "\$@"
+
+DOCKER_ARGS=(
+ -v "\${HOST_AIRFLOW_SOURCES}/tmp:/tmp" \\
+ -v "\${HOST_AIRFLOW_SOURCES}/files:/files" \\
+ -v "\${HOST_AIRFLOW_SOURCES}:/opt/airflow" \\
+ -v "\${HOST_HOME}/.aws:/root/.aws" \\
+ -v "\${HOST_HOME}/.azure:/root/.azure" \\
+ -v "\${HOST_HOME}/.config/gcloud:/root/.config/gcloud" \\
+ -w /opt/airflow \\
+ --env-file <(env | grep TF) \\
+ -e GOOGLE_APPLICATION_CREDENTIALS \\
+ -e CLOUDSDK_CONFIG \\
Review comment:
Instead of having separate functions we should have one with:
```
-v "\${HOST_AIRFLOW_SOURCES}/tmp:/tmp" \\
-v "\${HOST_AIRFLOW_SOURCES}/files:/files" \\
-v "\${HOST_AIRFLOW_SOURCES}:/opt/airflow" \\
-v "\${HOST_HOME}/.aws:/root/.aws" \\
-v "\${HOST_HOME}/.azure:/root/.azure" \\
-v "\${HOST_HOME}/.config/gcloud:/root/.config/gcloud" \\
-w /opt/airflow \\
--env-file <(env ) \\
```
That should do the job for all the tools (and any undocumented variables)
The:
```
--env-file <(env )
```
Will pass all the environment variables that are exported - making it equivalent to the way it is run locally.
----------------------------------------------------------------
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] mik-laj commented on a change in pull request #9219: Fix system tests for DataflowCreateJavaJobOperator
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #9219:
URL: https://github.com/apache/airflow/pull/9219#discussion_r438404044
##########
File path: scripts/ci/prepare_tool_scripts.sh
##########
@@ -29,19 +29,28 @@ function prepare_tool_script() {
cat >"${TARGET_TOOL_PATH}" <<EOF
#!/usr/bin/env bash
-docker run --rm -it \
- -v "\${HOST_AIRFLOW_SOURCES}/tmp:/tmp" \
- -v "\${HOST_AIRFLOW_SOURCES}/files:/files" \
- -v "\${HOST_AIRFLOW_SOURCES}:/opt/airflow" \
- -v "\${HOST_HOME}/${VOLUME}:/root/${VOLUME}" \
- "${IMAGE}" ${COMMAND} "\$@"
+DOCKER_ARGS=(
+ -v "\${HOST_AIRFLOW_SOURCES}/tmp:/tmp" \\
+ -v "\${HOST_AIRFLOW_SOURCES}/files:/files" \\
+ -v "\${HOST_AIRFLOW_SOURCES}:/opt/airflow" \\
+ -v "\${HOST_HOME}/${VOLUME}:/root/${VOLUME}" \\
+ --interactive \\
+)
+if [ -t 0 ] ; then
+ DOCKER_ARGS+=(
+ --tty \\
+ )
+fi
+
+docker run "\${DOCKER_ARGS[@]}" "${IMAGE}" "\${@}"
+
RES=\$?
if [[ \${HOST_OS} == "Linux" ]]; then
docker run --rm \
- -v "\${HOST_AIRFLOW_SOURCES}/tmp:/tmp" \
- -v "\${HOST_AIRFLOW_SOURCES}/files:/files" \
- -v "\${HOST_HOME}/${VOLUME}:/root/${VOLUME}" \
- "\${AIRFLOW_CI_IMAGE}" bash -c \
+ -v "\${HOST_AIRFLOW_SOURCES}/tmp:/tmp" \\
Review comment:
A single slash in a text string means that the newline is not displayed. We want a new line, so we need a double slash.
----------------------------------------------------------------
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] mik-laj commented on a change in pull request #9219: Fix system tests for DataflowCreateJavaJobOperator
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #9219:
URL: https://github.com/apache/airflow/pull/9219#discussion_r438404330
##########
File path: scripts/ci/prepare_tool_scripts.sh
##########
@@ -59,9 +68,60 @@ GCLOUD_IMAGE="gcr.io/google.com/cloudsdktool/cloud-sdk:latest"
prepare_tool_script "amazon/aws-cli:latest" ".aws" aws
prepare_tool_script "mcr.microsoft.com/azure-cli:latest" ".azure" az az
-prepare_tool_script "${GCLOUD_IMAGE}" ".config/gcloud" bq bq
-prepare_tool_script "${GCLOUD_IMAGE}" ".config/gcloud" gcloud gcloud
-prepare_tool_script "${GCLOUD_IMAGE}" ".config/gcloud" gsutil gsutil
+
+function prepare_gcloud_script() {
+ IMAGE="${1}"
+ TOOL="${2}"
+ COMMAND="${2}"
+ VOLUME=".config/gcloud"
+
+ TARGET_TOOL_PATH="/usr/bin/${TOOL}"
+ TARGET_TOOL_UPDATE_PATH="/usr/bin/${TOOL}-update"
+
+ cat >"${TARGET_TOOL_PATH}" <<EOF
+#!/usr/bin/env bash
+
+DOCKER_ARGS=(
+ --rm \\
+ -v "\${HOST_AIRFLOW_SOURCES}/tmp:/tmp" \\
+ -v "\${HOST_AIRFLOW_SOURCES}/files:/files" \\
+ -v "\${HOST_AIRFLOW_SOURCES}:/opt/airflow" \\
+ -v "\${HOST_HOME}/${VOLUME}:/root/${VOLUME}" \\
+ -e GOOGLE_APPLICATION_CREDENTIALS \\
+ -e CLOUDSDK_CONFIG \\
Review comment:
GOOGLE_APPLICATION_CREDENTIALS and CLOUDSDK_CONFIG are required by ADC. This second environment variable is unfortunately not officially documented in [the guide](https://cloud.google.com/docs/authentication/production)
----------------------------------------------------------------
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 #9219: Fix system tests for DataflowCreateJavaJobOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #9219:
URL: https://github.com/apache/airflow/pull/9219#discussion_r438434218
##########
File path: scripts/ci/prepare_tool_scripts.sh
##########
@@ -59,9 +68,60 @@ GCLOUD_IMAGE="gcr.io/google.com/cloudsdktool/cloud-sdk:latest"
prepare_tool_script "amazon/aws-cli:latest" ".aws" aws
prepare_tool_script "mcr.microsoft.com/azure-cli:latest" ".azure" az az
-prepare_tool_script "${GCLOUD_IMAGE}" ".config/gcloud" bq bq
-prepare_tool_script "${GCLOUD_IMAGE}" ".config/gcloud" gcloud gcloud
-prepare_tool_script "${GCLOUD_IMAGE}" ".config/gcloud" gsutil gsutil
+
+function prepare_gcloud_script() {
+ IMAGE="${1}"
+ TOOL="${2}"
+ COMMAND="${2}"
+ VOLUME=".config/gcloud"
+
+ TARGET_TOOL_PATH="/usr/bin/${TOOL}"
+ TARGET_TOOL_UPDATE_PATH="/usr/bin/${TOOL}-update"
+
+ cat >"${TARGET_TOOL_PATH}" <<EOF
+#!/usr/bin/env bash
+
+DOCKER_ARGS=(
+ --rm \\
+ -v "\${HOST_AIRFLOW_SOURCES}/tmp:/tmp" \\
+ -v "\${HOST_AIRFLOW_SOURCES}/files:/files" \\
+ -v "\${HOST_AIRFLOW_SOURCES}:/opt/airflow" \\
+ -v "\${HOST_HOME}/${VOLUME}:/root/${VOLUME}" \\
+ -e GOOGLE_APPLICATION_CREDENTIALS \\
+ -e CLOUDSDK_CONFIG \\
Review comment:
I think we should do it differently. We shoudl simplify all the scripts to the same form now - having separate script for each function defeats the purpose.
----------------------------------------------------------------
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] mik-laj commented on pull request #9219: Fix system tests for DataflowCreateJavaJobOperator
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #9219:
URL: https://github.com/apache/airflow/pull/9219#issuecomment-642808722
Substituted by. https://github.com/apache/airflow/pull/9223
----------------------------------------------------------------
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] mik-laj commented on a change in pull request #9219: Fix system tests for DataflowCreateJavaJobOperator
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #9219:
URL: https://github.com/apache/airflow/pull/9219#discussion_r438403264
##########
File path: scripts/ci/docker-compose/local-prod.yml
##########
@@ -35,7 +35,7 @@ services:
- ../../../setup.cfg:/opt/airflow/setup.cfg:cached
- ../../../setup.py:/opt/airflow/setup.py:cached
- ../../../tests:/opt/airflow/tests:cached
- - ../../../tmp:/opt/airflow/tmp:cached
+ - ../../../tmp:/tmp:cached
Review comment:
It was a huge problem for me. It took me a long time to find this bug. I did not know why the jar does not start, because for manual testing I had this file saved in /files/. It was very confusing. 😿
----------------------------------------------------------------
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 #9219: Fix system tests for DataflowCreateJavaJobOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #9219:
URL: https://github.com/apache/airflow/pull/9219#discussion_r438432062
##########
File path: scripts/ci/prepare_tool_scripts.sh
##########
@@ -29,19 +29,28 @@ function prepare_tool_script() {
cat >"${TARGET_TOOL_PATH}" <<EOF
#!/usr/bin/env bash
-docker run --rm -it \
- -v "\${HOST_AIRFLOW_SOURCES}/tmp:/tmp" \
- -v "\${HOST_AIRFLOW_SOURCES}/files:/files" \
- -v "\${HOST_AIRFLOW_SOURCES}:/opt/airflow" \
- -v "\${HOST_HOME}/${VOLUME}:/root/${VOLUME}" \
- "${IMAGE}" ${COMMAND} "\$@"
+DOCKER_ARGS=(
+ -v "\${HOST_AIRFLOW_SOURCES}/tmp:/tmp" \\
+ -v "\${HOST_AIRFLOW_SOURCES}/files:/files" \\
+ -v "\${HOST_AIRFLOW_SOURCES}:/opt/airflow" \\
+ -v "\${HOST_HOME}/${VOLUME}:/root/${VOLUME}" \\
+ --interactive \\
+)
+if [ -t 0 ] ; then
+ DOCKER_ARGS+=(
+ --tty \\
+ )
+fi
+
+docker run "\${DOCKER_ARGS[@]}" "${IMAGE}" "\${@}"
+
RES=\$?
if [[ \${HOST_OS} == "Linux" ]]; then
docker run --rm \
- -v "\${HOST_AIRFLOW_SOURCES}/tmp:/tmp" \
- -v "\${HOST_AIRFLOW_SOURCES}/files:/files" \
- -v "\${HOST_HOME}/${VOLUME}:/root/${VOLUME}" \
- "\${AIRFLOW_CI_IMAGE}" bash -c \
+ -v "\${HOST_AIRFLOW_SOURCES}/tmp:/tmp" \\
Review comment:
We dd not want a new line here - the idea is that it gets converted to a single line in generated scripts, But this way it will work as well. So no problem with 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #9219: Fix system tests for DataflowCreateJavaJobOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #9219:
URL: https://github.com/apache/airflow/pull/9219#discussion_r438434218
##########
File path: scripts/ci/prepare_tool_scripts.sh
##########
@@ -59,9 +68,60 @@ GCLOUD_IMAGE="gcr.io/google.com/cloudsdktool/cloud-sdk:latest"
prepare_tool_script "amazon/aws-cli:latest" ".aws" aws
prepare_tool_script "mcr.microsoft.com/azure-cli:latest" ".azure" az az
-prepare_tool_script "${GCLOUD_IMAGE}" ".config/gcloud" bq bq
-prepare_tool_script "${GCLOUD_IMAGE}" ".config/gcloud" gcloud gcloud
-prepare_tool_script "${GCLOUD_IMAGE}" ".config/gcloud" gsutil gsutil
+
+function prepare_gcloud_script() {
+ IMAGE="${1}"
+ TOOL="${2}"
+ COMMAND="${2}"
+ VOLUME=".config/gcloud"
+
+ TARGET_TOOL_PATH="/usr/bin/${TOOL}"
+ TARGET_TOOL_UPDATE_PATH="/usr/bin/${TOOL}-update"
+
+ cat >"${TARGET_TOOL_PATH}" <<EOF
+#!/usr/bin/env bash
+
+DOCKER_ARGS=(
+ --rm \\
+ -v "\${HOST_AIRFLOW_SOURCES}/tmp:/tmp" \\
+ -v "\${HOST_AIRFLOW_SOURCES}/files:/files" \\
+ -v "\${HOST_AIRFLOW_SOURCES}:/opt/airflow" \\
+ -v "\${HOST_HOME}/${VOLUME}:/root/${VOLUME}" \\
+ -e GOOGLE_APPLICATION_CREDENTIALS \\
+ -e CLOUDSDK_CONFIG \\
Review comment:
I thin we should do it differently. We shoudl simplify all the scripts to the same form now - having separate script for each command defeats the purpose.
----------------------------------------------------------------
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