[GitHub] [airflow] uranusjr edited a comment on pull request #16932: Adds option to disable mounting temporary folder in DockerOperator

[GitBox <gi...@apache.org>]

uranusjr edited a comment on pull request #16932:
URL: https://github.com/apache/airflow/pull/16932#issuecomment-880360742


   The cause to the failure is that old `binds` thing *automatically create an empty directory* if the host path does not exist; the new `mounts` thing does not do that, but fails loudly (a delibrate decision made by Docker to prevent subtle bugs, and this is actually one of that). So the previous implementation isn’t really working either, but broken in a different way, the apparently-mounted temporary directory was actually not able to be used when used in the Docker-in-Docker setup. So in a way this is catching a subtle bug that went unnoticed previously.
   
   So IMO your fix makes sense. The temporary directory thing won’t work for the Docker-in-Docker setup anyway, so an additional argument makes sense. This should also be recorded in the 2.0 changelog; not in the way proposed in #17008, but something like “previously the temporary directory mount was silently failing when [not sure what best to put here], now it will fail loudly and you must explicitly pass `mount_tmp_dir=False` when you don’t need the temporary directory”. And maybe we can also deprecate the implicit `mount_tmp_dir=True` behaviour and switch to *not* mount the temporary directory by default in 3.0.


-- 
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