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/04/07 02:41:47 UTC
[GitHub] [airflow] ouchl opened a new pull request #8169: [AIRFLOW-XXXX]
Minor improvement in Dockerfile
ouchl opened a new pull request #8169: [AIRFLOW-XXXX] Minor improvement in Dockerfile
URL: https://github.com/apache/airflow/pull/8169
---
Make sure to mark the boxes below before creating PR: [x]
- [ ] Description above provides context of the change
- [ ] Unit tests coverage for changes (not needed for documentation changes)
- [ ] Commits follow "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)"
- [ ] Relevant documentation is updated including usage instructions.
- [ ] 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
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #8169: [AIRFLOW-XXXX] Minor
improvement in Dockerfile
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #8169: [AIRFLOW-XXXX] Minor improvement in Dockerfile
URL: https://github.com/apache/airflow/pull/8169#issuecomment-610258682
I think that change might be actually more misleading.
The image is prepared in the way that it can be installed from different sources "." - local sources, PyPi, GitHub URL. Therfor AIRFLOW_SOURCES_FROM and AIRFLOW_INSTALL_SOURCES might have different forms - it can be "." (for local sources) but it also can be "apache-airflow" for installing from PyPI as well as "https://github.com/apache/airflow/archive/1.10.10.tar.gz#egg=apache-airflow"
Take for example:
```
CONSTRAINT_REQUIREMENTS=" "${AIRFLOW_SOURCES_FROM}/requirements/requirements-python${PYTHON_MAJOR_MINOR_VERSION}.txt"
```
This might suggest that it will work correctly when we just replace AIRFLOW_SOURCES_FROM with "apache-airflow" - which is not OK (you need to override COSTRAINT_REQUIREMENTS to point to URL in this case. For example:
```
--build-arg CONSTRAINT_REQUIREMENTS="https://raw.githubusercontent.com/apache/airflow/1.10.10/requirements/requirements-python3.7.txt"
```
Those arg should be set independently and there are different sets of those args that are valid - and there is no relation between them that is suggested by this change.
See examples here:
https://github.com/apache/airflow/blob/master/IMAGES.rst#production-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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] ouchl commented on issue #8169: [AIRFLOW-XXXX] Minor
improvement in Dockerfile
Posted by GitBox <gi...@apache.org>.
ouchl commented on issue #8169: [AIRFLOW-XXXX] Minor improvement in Dockerfile
URL: https://github.com/apache/airflow/pull/8169#issuecomment-611015247
> > @potiuk Thank you for your explanation. If it can be a URL why we are using
> > `COPY ${AIRFLOW_SOURCES_FROM} ${AIRFLOW_SOURCES_TO}`
> > instead of
> > `ADD ${AIRFLOW_SOURCES_FROM} ${AIRFLOW_SOURCES_TO}`
> > and AIRFLOW_SOURCES_FROM is used before definition, so it seems it will take no effect to change its value.
>
> Ah. good point. it's an interesting sequence and I think it's worth correcting. This is purely optimisation of image size when building it from a remote GitHub URL.
>
> `AIRFLOW_SOURCES_FROM` is to install airflow from local sources. In case we do not use Airflow Sources but GitHub URL we set it to "Dockerfile" so that we don't copy the whole context/sources (20MB) but just a small Dockerfile (which we know is there). and then we install it from remote. Otherwise we would copy all the airflow sources to the image and then install it from remote URL. So this is really an optimisation. And that's why COPY is OK because it is either "." or "Dockerfile"
Got it. We can use Dockerfile to indicate the source instead of copying the real sources. Thanks.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] potiuk edited a comment on issue #8169: [AIRFLOW-XXXX]
Minor improvement in Dockerfile
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on issue #8169: [AIRFLOW-XXXX] Minor improvement in Dockerfile
URL: https://github.com/apache/airflow/pull/8169#issuecomment-610258682
I think that change might be actually more misleading.
The image is prepared in the way that it can be installed from different sources "." - local sources, PyPi, GitHub URL. Therfor AIRFLOW_SOURCES_FROM and AIRFLOW_INSTALL_SOURCES might have different forms - it can be "." (for local sources) but it also can be "apache-airflow" for installing from PyPI as well as "https://github.com/apache/airflow/archive/1.10.10.tar.gz#egg=apache-airflow"
Take for example:
```
CONSTRAINT_REQUIREMENTS=" "${AIRFLOW_SOURCES_FROM}/requirements/requirements-python${PYTHON_MAJOR_MINOR_VERSION}.txt"
```
This might suggest that it will work correctly when we just replace AIRFLOW_SOURCES_FROM with "apache-airflow" - which is not OK (you need to override COSTRAINT_REQUIREMENTS to point to URL in this case. For example:
```
--build-arg CONSTRAINT_REQUIREMENTS="https://raw.githubusercontent.com/apache/airflow/1.10.10/requirements/requirements-python3.7.txt"
```
Those args should be set independently and there are different sets of those args that are valid - and there is no relation between them that is suggested by this change.
See examples here:
https://github.com/apache/airflow/blob/master/IMAGES.rst#production-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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] ouchl commented on issue #8169: [AIRFLOW-XXXX] Minor
improvement in Dockerfile
Posted by GitBox <gi...@apache.org>.
ouchl commented on issue #8169: [AIRFLOW-XXXX] Minor improvement in Dockerfile
URL: https://github.com/apache/airflow/pull/8169#issuecomment-610764307
> Please take a look @ouchl - but I think we should close this one.
@potiuk Thank you for your explanation. If it can be a URL why we are using
`COPY ${AIRFLOW_SOURCES_FROM} ${AIRFLOW_SOURCES_TO}`
instead of
`ADD ${AIRFLOW_SOURCES_FROM} ${AIRFLOW_SOURCES_TO}`
and AIRFLOW_SOURCES_FROM is used before definition, so it seems it will take no effect to change its value.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] ouchl closed pull request #8169: [AIRFLOW-XXXX] Minor
improvement in Dockerfile
Posted by GitBox <gi...@apache.org>.
ouchl closed pull request #8169: [AIRFLOW-XXXX] Minor improvement in Dockerfile
URL: https://github.com/apache/airflow/pull/8169
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #8169: [AIRFLOW-XXXX] Minor
improvement in Dockerfile
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #8169: [AIRFLOW-XXXX] Minor improvement in Dockerfile
URL: https://github.com/apache/airflow/pull/8169#issuecomment-610258963
Please take a look @ouchl - but I think we should close this one.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #8169: [AIRFLOW-XXXX] Minor
improvement in Dockerfile
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #8169: [AIRFLOW-XXXX] Minor improvement in Dockerfile
URL: https://github.com/apache/airflow/pull/8169#issuecomment-610900201
> @potiuk Thank you for your explanation. If it can be a URL why we are using
> `COPY ${AIRFLOW_SOURCES_FROM} ${AIRFLOW_SOURCES_TO}`
> instead of
> `ADD ${AIRFLOW_SOURCES_FROM} ${AIRFLOW_SOURCES_TO}`
> and AIRFLOW_SOURCES_FROM is used before definition, so it seems it will take no effect to change its value.
Ah. good point. it's an interesting sequence and I think it's worth correcting. This is purely optimisation of image size when building it from a remote GitHub URL.
`AIRFLOW_SOURCES_FROM` is to install airflow from local sources. In case we do not use Airflow Sources but GitHub URL we set it to "Dockerfile" so that we don't copy the whole context/sources (20MB) but just a small Dockerfile (which we know is there). and then we install it from remote. Otherwise we would copy all the airflow sources to the image and then install it from remote URL. So this is really an optimisation. And that's why COPY is OK because it is either "." or "Dockerfile"
----------------------------------------------------------------
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
With regards,
Apache Git Services