potiuk commented on code in PR #24871:
URL: https://github.com/apache/airflow/pull/24871#discussion_r916336469
##########
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.")
[email protected](
+ '--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:
See above - I think it's not an error.
One more comment - why: mainly because "fix-ownership" shoudl be used in CI
scripts. Which usually run a sequence of commands. And "fix-ownership" main
goal is to make sure that the checked out directory of Airlfow is in "pristine"
state" - i.e. no files have "root" ownership (because if they are then it
makes further check-outs potentially failing when run as non-root user).
So any kind of sequence of operations that we run in CI is:
1) checkout
2) do some stuff (potentially in docker producing root-owned files on linux)
3) fix-ownership
Result: -> "pristine" workspace, no "root owned" files
if we start to run our CI on Mac or Windows (we will likely eventually run
some CI jobs on Windows at the very least) , if you error out in step 3), you
have to do script like this:
1) checkout
2) do some stuff (potentially in docker producing root-owned files on linux)
3) if Linux: fix-ownership
I prefer to run the conditional inside fix-ownership command and NOT
error-out on windows. because otherwise i have to repeat "If linux" in all
places.
We already run fix-ownershio in 25 places in our CI
```
[jarek:~/code/airflow-copy] [airflow-3.9] v2-3-test+ ± grep fix-ownership
.github/workflows/*.yml | wc
25 100 1526
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
