[GitHub] [airflow] jedcunningham commented on a diff in pull request #24871: Run "fix-ownership" with sudo rather than docker image if specified

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

jedcunningham commented on code in PR #24871:
URL: https://github.com/apache/airflow/pull/24871#discussion_r916321910


##########
dev/breeze/src/airflow_breeze/commands/ci_commands.py:
##########
@@ -114,11 +127,68 @@ def resource_check(verbose: bool, dry_run: bool):
     check_docker_resources(shell_params.airflow_image_name, verbose=verbose, dry_run=dry_run)
 
 
+HOME_DIR = Path(os.path.expanduser('~')).resolve()
+
+DIRECTORIES_TO_FIX = [
+    AIRFLOW_SOURCES_ROOT,
+    HOME_DIR / ".aws",
+    HOME_DIR / ".azure",
+    HOME_DIR / ".config/gcloud",
+    HOME_DIR / ".docker",
+    AIRFLOW_SOURCES_ROOT,
+]
+
+
+def fix_ownership_for_file(file: Path, dry_run: bool, verbose: bool):
+    get_console().print(f"[info]Fixing ownership of {file}")
+    result = run_command(
+        ["sudo", "chown", f"{os.getuid}:{os.getgid()}", str(file.resolve())],
+        check=False,
+        stderr=subprocess.STDOUT,
+        dry_run=dry_run,
+        verbose=verbose,
+    )
+    if result.returncode != 0:
+        get_console().print(f"[warning]Could not fix ownership for {file}: {result.stdout}")
+
+
+def fix_ownership_for_path(path: Path, dry_run: bool, verbose: bool):
+    if path.is_dir():
+        for p in Path(path).rglob('*'):
+            if p.owner == 'root':
+                fix_ownership_for_file(p, dry_run=dry_run, verbose=verbose)
+    else:
+        if path.owner == 'root':
+            fix_ownership_for_file(path, dry_run=dry_run, verbose=verbose)
+
+
+def fix_ownership_without_docker(dry_run: bool, verbose: bool):
+    for directory_to_fix in DIRECTORIES_TO_FIX:
+        fix_ownership_for_path(directory_to_fix, dry_run=dry_run, verbose=verbose)
+
+
 @main.command(name="fix-ownership", help="Fix ownership of source files to be same as host user.")
+@click.option(
+    '--use-sudo',
+    is_flag=True,
+    help="Use sudo instead of docker image to fix the ownership. You need to be a `sudoer` to run it",
+    envvar='USE_SUDO',
+)
 @option_github_repository
 @option_verbose
 @option_dry_run
-def fix_ownership(github_repository: str, verbose: bool, dry_run: bool):
+def fix_ownership(github_repository: str, use_sudo: bool, verbose: bool, dry_run: bool):
+    system = platform.system().lower()
+    if system != 'linux':
+        get_console().print(
+            f"[warning]You should only need to run fix-ownership on Linux and your system is {system}"
+        )
+        sys.exit(0)

Review Comment:
   Should this be non-zero?



##########
dev/breeze/src/airflow_breeze/commands/ci_commands.py:
##########
@@ -114,11 +127,68 @@ def resource_check(verbose: bool, dry_run: bool):
     check_docker_resources(shell_params.airflow_image_name, verbose=verbose, dry_run=dry_run)
 
 
+HOME_DIR = Path(os.path.expanduser('~')).resolve()
+
+DIRECTORIES_TO_FIX = [
+    AIRFLOW_SOURCES_ROOT,
+    HOME_DIR / ".aws",
+    HOME_DIR / ".azure",
+    HOME_DIR / ".config/gcloud",
+    HOME_DIR / ".docker",
+    AIRFLOW_SOURCES_ROOT,
+]
+
+
+def fix_ownership_for_file(file: Path, dry_run: bool, verbose: bool):
+    get_console().print(f"[info]Fixing ownership of {file}")
+    result = run_command(
+        ["sudo", "chown", f"{os.getuid}:{os.getgid()}", str(file.resolve())],
+        check=False,
+        stderr=subprocess.STDOUT,
+        dry_run=dry_run,
+        verbose=verbose,
+    )
+    if result.returncode != 0:
+        get_console().print(f"[warning]Could not fix ownership for {file}: {result.stdout}")
+
+
+def fix_ownership_for_path(path: Path, dry_run: bool, verbose: bool):
+    if path.is_dir():
+        for p in Path(path).rglob('*'):
+            if p.owner == 'root':
+                fix_ownership_for_file(p, dry_run=dry_run, verbose=verbose)
+    else:
+        if path.owner == 'root':
+            fix_ownership_for_file(path, dry_run=dry_run, verbose=verbose)
+
+
+def fix_ownership_without_docker(dry_run: bool, verbose: bool):
+    for directory_to_fix in DIRECTORIES_TO_FIX:
+        fix_ownership_for_path(directory_to_fix, dry_run=dry_run, verbose=verbose)
+
+
 @main.command(name="fix-ownership", help="Fix ownership of source files to be same as host user.")
+@click.option(
+    '--use-sudo',
+    is_flag=True,
+    help="Use sudo instead of docker image to fix the ownership. You need to be a `sudoer` to run it",
+    envvar='USE_SUDO',
+)
 @option_github_repository
 @option_verbose
 @option_dry_run
-def fix_ownership(github_repository: str, verbose: bool, dry_run: bool):
+def fix_ownership(github_repository: str, use_sudo: bool, verbose: bool, dry_run: bool):
+    system = platform.system().lower()
+    if system != 'linux':
+        get_console().print(
+            f"[warning]You should only need to run fix-ownership on Linux and your system is {system}"

Review Comment:
   Not sure this is a warning either. Error maybe?



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