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/03 22:13:11 UTC
[GitHub] [airflow] potiuk opened a new pull request #11262: The scirpt for CI image is now place in the docker folder
potiuk opened a new pull request #11262:
URL: https://github.com/apache/airflow/pull/11262
The script was previously placed in scripts/ci which caused
a bit of a problem in 1-10-test branch where PRs were using
scripts/ci from the v1-10-test HEAD but they were missing
the ci script from the PR.
---
**^ 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 #11262: The bats script for CI image is now placed in the docker folder
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #11262:
URL: https://github.com/apache/airflow/pull/11262#issuecomment-703172766
> Could you please also remove `!scripts/ci/dockerfiles/bats` from dockerignore, I added it for this script only.
Yep. will do.
----------------------------------------------------------------
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] OmairK commented on pull request #11262: The bats script for CI image is now placed in the docker folder
Posted by GitBox <gi...@apache.org>.
OmairK commented on pull request #11262:
URL: https://github.com/apache/airflow/pull/11262#issuecomment-703172647
Could you please also remove `!scripts/ci/dockerfiles/bats` from dockerignore, I added it for this script only.
----------------------------------------------------------------
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 #11262: The bats script for CI image is now placed in the docker folder
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #11262:
URL: https://github.com/apache/airflow/pull/11262#discussion_r499187391
##########
File path: Dockerfile.ci
##########
@@ -216,7 +216,7 @@ RUN mkdir -p /opt/bats/lib/bats-file \
RUN echo "export PATH=/opt/bats/bin:${PATH}" >> /root/.bashrc
# Additional scripts for managing BATS addons
-COPY scripts/ci/dockerfiles/bats/load.bash /opt/bats/lib/
Review comment:
I've added a bit more explanation on that in the commit
----------------------------------------------------------------
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 #11262: The bats script for CI image is now placed in the docker folder
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #11262:
URL: https://github.com/apache/airflow/pull/11262
----------------------------------------------------------------
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 #11262: The scirpt for CI image is now place in the docker folder
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #11262:
URL: https://github.com/apache/airflow/pull/11262#issuecomment-703171051
cc: @OmairK @paolaperaza
----------------------------------------------------------------
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 #11262: The bats script for CI image is now placed in the docker folder
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #11262:
URL: https://github.com/apache/airflow/pull/11262#issuecomment-703172945
Removed @OmairK . Good catch!
----------------------------------------------------------------
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 #11262: The bats script for CI image is now placed in the docker folder
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #11262:
URL: https://github.com/apache/airflow/pull/11262#discussion_r499187247
##########
File path: Dockerfile.ci
##########
@@ -216,7 +216,7 @@ RUN mkdir -p /opt/bats/lib/bats-file \
RUN echo "export PATH=/opt/bats/bin:${PATH}" >> /root/.bashrc
# Additional scripts for managing BATS addons
-COPY scripts/ci/dockerfiles/bats/load.bash /opt/bats/lib/
Review comment:
No. It is needed in the "local" bats. It's a bit of duplication (but very little) but it helps to keep stuff in order. The scripts in "ci" are those that are used in the host, where the scripts in "docker" are needed by the Docker image to be used as part of the build context.
The problem is that in the workflow run the "ci" scripts for security come always from master (so that no-one can abuse the GITHUB_TOKEN with write access, but the Dockerfile.ci and all the stuff that is needed to build the Docker image needs to come from the PR.
I know it's not straightforward :( . But this way we have huge optimisations when building the image only once in workflow_run,
----------------------------------------------------------------
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 #11262: The bats script for CI image is now placed in the docker folder
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #11262:
URL: https://github.com/apache/airflow/pull/11262#discussion_r499186447
##########
File path: Dockerfile.ci
##########
@@ -216,7 +216,7 @@ RUN mkdir -p /opt/bats/lib/bats-file \
RUN echo "export PATH=/opt/bats/bin:${PATH}" >> /root/.bashrc
# Additional scripts for managing BATS addons
-COPY scripts/ci/dockerfiles/bats/load.bash /opt/bats/lib/
Review comment:
Should we remove `scripts/ci/dockerfiles/bats/load.bash` 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org