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 2022/06/30 21:28:10 UTC
[GitHub] [airflow] potiuk commented on a diff in pull request #24751: Adding Docker context check for breeze
potiuk commented on code in PR #24751:
URL: https://github.com/apache/airflow/pull/24751#discussion_r911454707
##########
dev/breeze/src/airflow_breeze/utils/docker_command_utils.py:
##########
@@ -299,6 +299,38 @@ def check_docker_compose_version(verbose: bool):
)
+def check_docker_context(verbose: bool):
+ """
+ Checks whether Docker is using the expected context
+ :param verbose: print commands when running
+ """
+ expected_docker_context = "default"
+ response = run_command(
+ ["docker", "info", "--format", "{{json .ClientInfo.Context}}"],
+ verbose=verbose,
+ no_output_dump_on_exception=False,
+ text=True,
+ capture_output=True,
+ )
+ if response.returncode != 0:
+ get_console().print(
+ '[error]Could not check for Docker context.[/]\n'
Review Comment:
I'd return into warning and immediately return from the function rather than print error. Generally as a rule "error" is reserved for some things that the user should fix to continue.
We have this ADR written describing it: https://airflowsummit.org/sessions/2021/dynamic-security-roles-in-airflow-for-multi-tenancy/
In this case, if we cannot check docker, I think this might mean for example that for whatever reason docker "client' we have does not provide this information (but in this case I'd say things should work in general - so likely we can continue).
The following warning is also a bit misleading - what does it mean "installed and running properly" ? The message should provide some helpful hints to the user - for example "properly (`docker info` should show Context: default")
##########
dev/breeze/src/airflow_breeze/utils/docker_command_utils.py:
##########
@@ -299,6 +299,38 @@ def check_docker_compose_version(verbose: bool):
)
+def check_docker_context(verbose: bool):
+ """
+ Checks whether Docker is using the expected context
+ :param verbose: print commands when running
+ """
+ expected_docker_context = "default"
+ response = run_command(
+ ["docker", "info", "--format", "{{json .ClientInfo.Context}}"],
+ verbose=verbose,
+ no_output_dump_on_exception=False,
+ text=True,
+ capture_output=True,
+ )
+ if response.returncode != 0:
+ get_console().print(
+ '[error]Could not check for Docker context.[/]\n'
+ '[warning]Please make sure Docker is installed and running properly.[/]'
+ )
+
Review Comment:
as discused above - I think we should return here. Otherwise we are passing srdout of command that failed to parse the context - which might have strange results. What happen if stdout is None (I guess this wil happen if the process did not even manage to start) - but not sure about it. Regardless return here is better.
##########
dev/breeze/src/airflow_breeze/utils/docker_command_utils.py:
##########
@@ -299,6 +299,38 @@ def check_docker_compose_version(verbose: bool):
)
+def check_docker_context(verbose: bool):
+ """
+ Checks whether Docker is using the expected context
+ :param verbose: print commands when running
+ """
+ expected_docker_context = "default"
+ response = run_command(
+ ["docker", "info", "--format", "{{json .ClientInfo.Context}}"],
+ verbose=verbose,
+ no_output_dump_on_exception=False,
+ text=True,
+ capture_output=True,
+ )
+ if response.returncode != 0:
+ get_console().print(
+ '[error]Could not check for Docker context.[/]\n'
+ '[warning]Please make sure Docker is installed and running properly.[/]'
+ )
+
+ used_docker_context = response.stdout.strip().replace('"', '')
+
+ if used_docker_context == expected_docker_context:
+ get_console().print(f'[success]Good Docker context used: {used_docker_context}.[/]')
+ else:
+ get_console().print(
+ f'[error]Docker is not using the default context, used context is: {used_docker_context}[/]\n'
+ f'[warning]Please make sure Docker is using the {expected_docker_context} context.[/]\n'
+ f'[warning]You can try switching contexts by running: "docker context use '
+ f'{expected_docker_context}"[/]'
+ )
+
Review Comment:
We should `sys.exit(1)` here. We know it makes no sense to continue. Save breeze the pain of trying fruitlessly to continue and kill it mercifully and immediately.
##########
dev/breeze/src/airflow_breeze/utils/docker_command_utils.py:
##########
@@ -299,6 +299,38 @@ def check_docker_compose_version(verbose: bool):
)
+def check_docker_context(verbose: bool):
+ """
+ Checks whether Docker is using the expected context
+ :param verbose: print commands when running
+ """
+ expected_docker_context = "default"
+ response = run_command(
+ ["docker", "info", "--format", "{{json .ClientInfo.Context}}"],
+ verbose=verbose,
+ no_output_dump_on_exception=False,
+ text=True,
+ capture_output=True,
+ )
+ if response.returncode != 0:
+ get_console().print(
+ '[error]Could not check for Docker context.[/]\n'
+ '[warning]Please make sure Docker is installed and running properly.[/]'
+ )
+
+ used_docker_context = response.stdout.strip().replace('"', '')
+
+ if used_docker_context == expected_docker_context:
+ get_console().print(f'[success]Good Docker context used: {used_docker_context}.[/]')
+ else:
+ get_console().print(
+ f'[error]Docker is not using the default context, used context is: {used_docker_context}[/]\n'
+ f'[warning]Please make sure Docker is using the {expected_docker_context} context.[/]\n'
+ f'[warning]You can try switching contexts by running: "docker context use '
Review Comment:
Nice.
--
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