[GitHub] [airflow] potiuk commented on a change in pull request #7724: [AIRFLOW-1536] Inherit umask from parent process in daemon mode
potiuk commented on a change in pull request #7724: [AIRFLOW-1536] Inherit umask from parent process in daemon mode URL: https://github.com/apache/airflow/pull/7724#discussion_r392692428 ## File path: airflow/cli/commands/celery_command.py ## @@ -135,6 +135,7 @@ def worker(args): ctx = daemon.DaemonContext( files_preserve=[handle], +umask=cli_utils.get_umask(), Review comment: Just to give some example - there are problems with Dockerfile caching when git by default uses umask of the system rather than have predefined value. Different systems at different times had different default umask (for example debian/mint had it different and different versions) and that means that your daemon might behave differently for those different systems, 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [airflow] potiuk commented on a change in pull request #7724: [AIRFLOW-1536] Inherit umask from parent process in daemon mode
potiuk commented on a change in pull request #7724: [AIRFLOW-1536] Inherit umask from parent process in daemon mode URL: https://github.com/apache/airflow/pull/7724#discussion_r392692428 ## File path: airflow/cli/commands/celery_command.py ## @@ -135,6 +135,7 @@ def worker(args): ctx = daemon.DaemonContext( files_preserve=[handle], +umask=cli_utils.get_umask(), Review comment: Just to give some example - there are problems with Dockerfile caching when git by default uses umask of the systems (for example debian/mint had it different and different versions) rather than have predefined value. Different systems at different times had different default umask and that means that your daemon might behave differently for those different systems, 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [airflow] potiuk commented on a change in pull request #7724: [AIRFLOW-1536] Inherit umask from parent process in daemon mode
potiuk commented on a change in pull request #7724: [AIRFLOW-1536] Inherit umask from parent process in daemon mode URL: https://github.com/apache/airflow/pull/7724#discussion_r392692256 ## File path: airflow/cli/commands/celery_command.py ## @@ -135,6 +135,7 @@ def worker(args): ctx = daemon.DaemonContext( files_preserve=[handle], +umask=cli_utils.get_umask(), Review comment: The problem is that you do not know what the umask is in the parent process - it depends on the system umask setting. It's a nuance but I think it's a good practice to have it fixed. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [airflow] potiuk commented on a change in pull request #7724: [AIRFLOW-1536] Inherit umask from parent process in daemon mode
potiuk commented on a change in pull request #7724: [AIRFLOW-1536] Inherit
umask from parent process in daemon mode
URL: https://github.com/apache/airflow/pull/7724#discussion_r392612429
##
File path: airflow/utils/cli.py
##
@@ -238,3 +238,12 @@ def sigquit_handler(sig, frame): # pylint:
disable=unused-argument
if line:
code.append(" {}".format(line.strip()))
print("\n".join(code))
+
+
+def get_umask():
+"""
+Returns umask to control default file permission for new files
+"""
+cur_mask = os.umask(0)
+os.umask(cur_mask)
Review comment:
I see - then it's yet another reason why we should not set/reset the umask.
We really do not need to find out what is the current umask - we should set a
reasonable value from configuration.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on a change in pull request #7724: [AIRFLOW-1536] Inherit umask from parent process in daemon mode
potiuk commented on a change in pull request #7724: [AIRFLOW-1536] Inherit
umask from parent process in daemon mode
URL: https://github.com/apache/airflow/pull/7724#discussion_r392602040
##
File path: airflow/utils/cli.py
##
@@ -238,3 +238,12 @@ def sigquit_handler(sig, frame): # pylint:
disable=unused-argument
if line:
code.append(" {}".format(line.strip()))
print("\n".join(code))
+
+
+def get_umask():
+"""
+Returns umask to control default file permission for new files
+"""
+cur_mask = os.umask(0)
+os.umask(cur_mask)
Review comment:
Why do you set it here ? I think this is not necessary.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on a change in pull request #7724: [AIRFLOW-1536] Inherit umask from parent process in daemon mode
potiuk commented on a change in pull request #7724: [AIRFLOW-1536] Inherit umask from parent process in daemon mode URL: https://github.com/apache/airflow/pull/7724#discussion_r392602337 ## File path: airflow/cli/commands/celery_command.py ## @@ -135,6 +135,7 @@ def worker(args): ctx = daemon.DaemonContext( files_preserve=[handle], +umask=cli_utils.get_umask(), Review comment: I think rather than reading from a parent we should allow to set it to a fixed value from configuration (with a reasonable default - for example 0o077). From the documentation of umask parameter in DaemonContext: ``` File access creation mask (“umask”) to set for the process on daemon start. A daemon should not rely on the parent process's umask value, which is beyond its control and may prevent creating a file with the required access mode. So when the daemon context opens, the umask is set to an explicit known value. If the conventional value of 0 is too open, consider setting a value such as 0o022, 0o027, 0o077, or another specific value. Otherwise, ensure the daemon creates every file with an explicit access mode for the purpose. ``` via https://kite.com/python/docs/daemon.DaemonContext 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: us...@infra.apache.org With regards, Apache Git Services
