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/04/18 08:20:36 UTC
[GitHub] [airflow] Bowrna opened a new pull request, #23052: Breeze adding exec command
Bowrna opened a new pull request, #23052:
URL: https://github.com/apache/airflow/pull/23052
closes: #21104
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
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
[GitHub] [airflow] Bowrna commented on a diff in pull request #23052: Breeze adding exec command
Posted by GitBox <gi...@apache.org>.
Bowrna commented on code in PR #23052:
URL: https://github.com/apache/airflow/pull/23052#discussion_r855166699
##########
dev/breeze/src/airflow_breeze/shell/enter_shell.py:
##########
@@ -206,3 +206,31 @@ def enter_shell(**kwargs):
console.print(CHEATSHEET, style=CHEATSHEET_STYLE)
enter_shell_params = ShellParams(**filter_out_none(**updated_kwargs))
run_shell_with_build_image_checks(verbose, dry_run, enter_shell_params)
+
+
+def find_airflow_container(**kwargs):
+ verbose = kwargs['verbose']
+ dry_run = kwargs['dry_run']
+ updated_kwargs = synchronize_cached_params(kwargs)
Review Comment:
ok @potiuk I will skip that part
--
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
[GitHub] [airflow] github-actions[bot] commented on pull request #23052: Breeze adding exec command
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #23052:
URL: https://github.com/apache/airflow/pull/23052#issuecomment-1112580980
The PR is likely ready to be merged. No tests are needed as no important environment files, nor python files were modified by it. However, committers might decide that full test matrix is needed and add the 'full tests needed' label. Then you should rebase it to the latest main or amend the last commit of the PR, and push it with --force-with-lease.
--
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
[GitHub] [airflow] potiuk commented on pull request #23052: Breeze adding exec command
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23052:
URL: https://github.com/apache/airflow/pull/23052#issuecomment-1108782475
The breeze.py looks like this now :).
```
from airflow_breeze.commands.configure_rich_click import click # noqa
from airflow_breeze.commands.main import main
from airflow_breeze.utils.path_utils import find_airflow_sources_root_to_operate_on
find_airflow_sources_root_to_operate_on()
if __name__ == '__main__':
main()
```
--
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
[GitHub] [airflow] potiuk commented on pull request #23052: Breeze adding exec command
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23052:
URL: https://github.com/apache/airflow/pull/23052#issuecomment-1108781294
It should be added in https://github.com/apache/airflow/blob/main/dev/breeze/src/airflow_breeze/commands/developer_commands.py - that's where all regular "development" commands were moved.
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #23052: Breeze adding exec command
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #23052:
URL: https://github.com/apache/airflow/pull/23052#discussion_r853029335
##########
dev/breeze/src/airflow_breeze/breeze.py:
##########
@@ -1486,5 +1487,47 @@ def __init__(self, **kwargs):
width=os.environ.get('RECORD_BREEZE_WIDTH'),
)
+
+@main.command(name='exec')
+@option_verbose
+@option_dry_run
+@click.argument('exec_args', nargs=-1, type=click.UNPROCESSED)
+def exec(
+ verbose: bool, dry_run: bool, exec_args: Tuple
+):
+ dc_run_file = BUILD_CACHE_DIR / DOCKER_COMPOSE_RUN_SCRIPT_FOR_CI
+ params = BuildCiParams()
+ ci_image_name = params.airflow_image_name
+ check_docker_resources(verbose, ci_image_name)
+ cmd = [str(dc_run_file), 'ps']
Review Comment:
Not sure entirely ;) . Docker and Docker compose filters are pretty weakly documented. But filter "give me container of the `aiflow` service is what we are looking for. In the `base.yml` we have:
```
version: "3.7"
services:
airflow:
image: ${AIRFLOW_CI_IMAGE_WITH_TAG}
```
So our container for airflow will always run with "airflow" service.
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #23052: Breeze adding exec command
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #23052:
URL: https://github.com/apache/airflow/pull/23052#discussion_r852910822
##########
dev/breeze/src/airflow_breeze/breeze.py:
##########
@@ -1486,5 +1487,47 @@ def __init__(self, **kwargs):
width=os.environ.get('RECORD_BREEZE_WIDTH'),
)
+
+@main.command(name='exec')
+@option_verbose
+@option_dry_run
+@click.argument('exec_args', nargs=-1, type=click.UNPROCESSED)
+def exec(
+ verbose: bool, dry_run: bool, exec_args: Tuple
+):
+ dc_run_file = BUILD_CACHE_DIR / DOCKER_COMPOSE_RUN_SCRIPT_FOR_CI
+ params = BuildCiParams()
+ ci_image_name = params.airflow_image_name
+ check_docker_resources(verbose, ci_image_name)
+ cmd = [str(dc_run_file), 'ps']
Review Comment:
We should really run `docker-compose ps` here. The bash attempted to use the generated dc_run file which is not needed any more,
But what we need we need exactly the same parameters that are used by ShellParams in shell commmand. Otherwise `docker-compose` does not know which file and which configuration should be used.
When we run docker-compose in Shell command we add :+1:
- COMPOSE_FILE variable
- Some environment variables for PORTs etc.
only when we use the same parameters and COMPOSE_FILE that we used for "breeze shell" we will be able to find running processes with "docker-compose ps" command.
So in short:
* don't use dc_run (it's the way docker compose command was run in Breeze)
* run docker-compose same way as in "shell" command (with the same env variables etc).
* and be a bit smarter when finding containers. I have not used the right filterring when I run the command , but it can be done a bit "smarter" with `--filter` command to only filter running `airflow` container.
* in order to test all the filtering, it might actually be nice to implement `docker-compose` command from the old Breeze. This command started docker-compose (same way as Shell command) and passed all the extra arguments passed directly to docker-compose command - this might be nice way to test various filtering strings.
--
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
[GitHub] [airflow] potiuk merged pull request #23052: Breeze adding exec command
Posted by GitBox <gi...@apache.org>.
potiuk merged PR #23052:
URL: https://github.com/apache/airflow/pull/23052
--
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
[GitHub] [airflow] potiuk commented on pull request #23052: Breeze adding exec command
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23052:
URL: https://github.com/apache/airflow/pull/23052#issuecomment-1105203589
> I will fix the changes that you have suggested @potiuk I have added docker-compose filter option to get the container id
Yep. Looks much better with the filter :)
--
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
[GitHub] [airflow] Bowrna commented on a diff in pull request #23052: Breeze adding exec command
Posted by GitBox <gi...@apache.org>.
Bowrna commented on code in PR #23052:
URL: https://github.com/apache/airflow/pull/23052#discussion_r853014899
##########
dev/breeze/src/airflow_breeze/breeze.py:
##########
@@ -1486,5 +1487,47 @@ def __init__(self, **kwargs):
width=os.environ.get('RECORD_BREEZE_WIDTH'),
)
+
+@main.command(name='exec')
+@option_verbose
+@option_dry_run
+@click.argument('exec_args', nargs=-1, type=click.UNPROCESSED)
+def exec(
+ verbose: bool, dry_run: bool, exec_args: Tuple
+):
+ dc_run_file = BUILD_CACHE_DIR / DOCKER_COMPOSE_RUN_SCRIPT_FOR_CI
+ params = BuildCiParams()
+ ci_image_name = params.airflow_image_name
+ check_docker_resources(verbose, ci_image_name)
+ cmd = [str(dc_run_file), 'ps']
Review Comment:
@potiuk Could you tell me how i can filter running `airflow` container? I can filter the running container, but specifically to get only the `airflow` container which property of the running container i could rely upon.
--
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
[GitHub] [airflow] potiuk commented on pull request #23052: Breeze adding exec command
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23052:
URL: https://github.com/apache/airflow/pull/23052#issuecomment-1102511594
> > > @potiuk Could you explain to me what will happen in this command? I tried executing `docker-compose ps` directly via terminal but it throws the following error.
> >
> >
> > `docker-compose ps` lists the containers status in a nicely formatted way. The naming probably comes from the command `ps` which shows the `process status`
> > The error states that it cannot find any configuration file. Check that you are running it in the right location where it can discover the config.
>
> thanks @joppevos yes `docker-compose ps` works when executed via `dc_run_file ps` but it doesn't work when executed directly. I tried from airflow_home and airflow_home/.build folder. Let me check other paths too.
That's becode dc_run_file from the old breeze already contains COMPOSE_FILE and all the environment variables that docker-compose needs to find out the definition of Breeze containers (see the other comment). This is precisely what we do in Shell command when we launch "docker-compose" - se we should use the same mechanism as in "shell" to get the variables and run docker-compose with all of them set.
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #23052: Breeze adding exec command
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #23052:
URL: https://github.com/apache/airflow/pull/23052#discussion_r855081480
##########
BREEZE.rst:
##########
@@ -549,12 +548,11 @@ command but it is very similar to current ``breeze`` command):
Another way is to exec into Breeze terminal from the host's terminal. Often you can
have multiple terminals in the host (Linux/MacOS/WSL2 on Windows) and you can simply use those terminals
-to enter the running container. It's as easy as launching ``./breeze-legacy exec`` while you already started the
+to enter the running container. It's as easy as launching ``./breeze exec`` while you already started the
Review Comment:
```suggestion
to enter the running container. It's as easy as launching ``breeze exec`` while you already started the
```
##########
BREEZE.rst:
##########
@@ -566,6 +564,12 @@ command and it is not yet available in the current ``breeze`` command):
</div>
+Those are all available flags of ``build-prod-image`` command:
Review Comment:
```suggestion
Those are all available flags of ``exec`` command:
```
##########
dev/breeze/src/airflow_breeze/breeze.py:
##########
@@ -1486,5 +1490,35 @@ def __init__(self, **kwargs):
width=os.environ.get('RECORD_BREEZE_WIDTH'),
)
+
+@main.command(
+ name='exec',
+ help='Joins the interactive shell of running airflow container'
+ )
+@option_verbose
+@option_dry_run
+@click.argument('exec_args', nargs=-1, type=click.UNPROCESSED)
+def exec(verbose: bool, dry_run: bool, exec_args: Tuple):
+ container_running = find_airflow_container(verbose=verbose, dry_run=dry_run)
+ cmd_to_run = [
+ "docker",
+ "exec",
+ "-it",
+ container_running,
+ "/opt/airflow/scripts/docker/entrypoint_exec.sh",
+ ]
+
+ if exec_args:
+ cmd_to_run.extend(exec_args)
+ run_command(
+ cmd_to_run,
+ verbose=verbose,
+ dry_run=dry_run,
+ check=False,
+ no_output_dump_on_exception=True,
Review Comment:
```suggestion
no_output_dump_on_exception=False,
```
I tihnk this is useful if exec fails. I also added a little better handling in case of some run_commands - with "nicer" message but I will handle that when I merge my PR.
##########
dev/breeze/src/airflow_breeze/breeze.py:
##########
@@ -1486,5 +1490,35 @@ def __init__(self, **kwargs):
width=os.environ.get('RECORD_BREEZE_WIDTH'),
)
+
+@main.command(
+ name='exec',
+ help='Joins the interactive shell of running airflow container'
+ )
+@option_verbose
+@option_dry_run
+@click.argument('exec_args', nargs=-1, type=click.UNPROCESSED)
+def exec(verbose: bool, dry_run: bool, exec_args: Tuple):
Review Comment:
Docstring is needed here to be displayed in help.
##########
dev/breeze/src/airflow_breeze/shell/enter_shell.py:
##########
@@ -206,3 +206,31 @@ def enter_shell(**kwargs):
console.print(CHEATSHEET, style=CHEATSHEET_STYLE)
enter_shell_params = ShellParams(**filter_out_none(**updated_kwargs))
run_shell_with_build_image_checks(verbose, dry_run, enter_shell_params)
+
+
+def find_airflow_container(**kwargs):
+ verbose = kwargs['verbose']
+ dry_run = kwargs['dry_run']
+ updated_kwargs = synchronize_cached_params(kwargs)
Review Comment:
No need to do that - synchronize is only needed for "python", "backend", "mysql_version" etc. - all those parameters that are actually kept in cache (and I have an idea how to get rid of the synchronize method altogether - I will do it right after I merge the big PR of mine for build images.
##########
dev/breeze/src/airflow_breeze/global_constants.py:
##########
@@ -33,6 +33,8 @@
DEFAULT_PYTHON_MAJOR_MINOR_VERSION = '3.7'
DEFAULT_BACKEND = 'sqlite'
+DOCKER_COMPOSE_RUN_SCRIPT_FOR_CI = "dc_ci"
Review Comment:
We can rmove that
##########
dev/breeze/src/airflow_breeze/shell/enter_shell.py:
##########
@@ -206,3 +206,31 @@ def enter_shell(**kwargs):
console.print(CHEATSHEET, style=CHEATSHEET_STYLE)
enter_shell_params = ShellParams(**filter_out_none(**updated_kwargs))
run_shell_with_build_image_checks(verbose, dry_run, enter_shell_params)
+
+
+def find_airflow_container(**kwargs):
Review Comment:
Better to pass the `verbose` and `dry_run` explicitly as parameters. no need to go via kwargs.
##########
dev/breeze/src/airflow_breeze/breeze.py:
##########
@@ -265,6 +265,9 @@
],
},
],
+ "breeze exec": [
Review Comment:
I tihnk this logically goes after `start-airflow` and before `stop` command so you should move it up a bit.
--
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
[GitHub] [airflow] potiuk commented on pull request #23052: Breeze adding exec command
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23052:
URL: https://github.com/apache/airflow/pull/23052#issuecomment-1105099543
> @potiuk Do I need to make any modifications in this PR wrt your current changes?
Not really - I think the only change that I applied that woudl affect this one is a little bit "nicer" handling of run_command results: when check = False but we actually care about the success/failure (for example when I run parallel runs). I think in this case just printing output is fine (I commented it) as exec is really 'interactive only' but it would be nice to catch the process return code and sys.exit() with it:
smth like:
```
process = run_command()
if not process:
sys.exit(1)
sys,exit(process.returncode)
```
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #23052: Breeze adding exec command
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #23052:
URL: https://github.com/apache/airflow/pull/23052#discussion_r858712727
##########
dev/breeze/src/airflow_breeze/commands/developer_commands.py:
##########
@@ -144,6 +144,11 @@
],
},
],
+ "breeze exec": [
Review Comment:
You should also add "exec" to developer commands above as it otherwise is spearated out:
I also unified all the "commands and parameters to have "." at the end of help.
--
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
[GitHub] [airflow] Bowrna commented on pull request #23052: Breeze adding exec command
Posted by GitBox <gi...@apache.org>.
Bowrna commented on PR #23052:
URL: https://github.com/apache/airflow/pull/23052#issuecomment-1104907562
@potiuk Do I need to make any modifications in this PR wrt your current changes?
--
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
[GitHub] [airflow] Bowrna commented on pull request #23052: Breeze adding exec command
Posted by GitBox <gi...@apache.org>.
Bowrna commented on PR #23052:
URL: https://github.com/apache/airflow/pull/23052#issuecomment-1109605533
@potiuk After rebasing the code, the code doesn't work properly. `docker-compose ps` doesn't list the airflow breeze container. I tried running with the old breeze and there issue arises due to the platform and sending the platform explicitly as `linux/amd64` also doesn't fix the process. I tried running via the `dc_ci` script in .build folder and it doesn't list the active breeze container.
Could you help me to find the issue?
--
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
[GitHub] [airflow] Bowrna commented on pull request #23052: Breeze adding exec command
Posted by GitBox <gi...@apache.org>.
Bowrna commented on PR #23052:
URL: https://github.com/apache/airflow/pull/23052#issuecomment-1102477867
> > @potiuk Could you explain to me what will happen in this command? I tried executing `docker-compose ps` directly via terminal but it throws the following error.
>
> `docker-compose ps` lists the containers status in a nicely formatted way. The naming probably comes from the command `ps` which shows the `process status`
>
> The error states that it cannot find any configuration file. Check that you are running it in the right location where it can discover the config.
thanks @joppevos yes `docker-compose ps` works when executed via `dc_run_file ps` but it doesn't work when executed directly. I tried from airflow_home and airflow_home/.build folder. Let me check other paths too.
--
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
[GitHub] [airflow] Bowrna commented on pull request #23052: Breeze adding exec command
Posted by GitBox <gi...@apache.org>.
Bowrna commented on PR #23052:
URL: https://github.com/apache/airflow/pull/23052#issuecomment-1101311816
@potiuk Could you explain me what will happen in this command?
https://github.com/apache/airflow/blob/3a2eb961ca628d962ed5fad68760ef3439b2f40b/breeze-legacy#L3512-L3513
Here `dc_run_file` indicates the path. Do i have to execute the `"${dc_run_file}" ps` via subprocess to get the process id?
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #23052: Breeze adding exec command
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #23052:
URL: https://github.com/apache/airflow/pull/23052#discussion_r853029335
##########
dev/breeze/src/airflow_breeze/breeze.py:
##########
@@ -1486,5 +1487,47 @@ def __init__(self, **kwargs):
width=os.environ.get('RECORD_BREEZE_WIDTH'),
)
+
+@main.command(name='exec')
+@option_verbose
+@option_dry_run
+@click.argument('exec_args', nargs=-1, type=click.UNPROCESSED)
+def exec(
+ verbose: bool, dry_run: bool, exec_args: Tuple
+):
+ dc_run_file = BUILD_CACHE_DIR / DOCKER_COMPOSE_RUN_SCRIPT_FOR_CI
+ params = BuildCiParams()
+ ci_image_name = params.airflow_image_name
+ check_docker_resources(verbose, ci_image_name)
+ cmd = [str(dc_run_file), 'ps']
Review Comment:
Not sure entirely ;) . Docker and Docker compose filters are pretty weakly documented. But filter "give me container of the `aiflow` service" is what we are looking for. In the `base.yml` we have:
```
version: "3.7"
services:
airflow:
image: ${AIRFLOW_CI_IMAGE_WITH_TAG}
```
So our container for airflow will always run with "airflow" service.
##########
dev/breeze/src/airflow_breeze/breeze.py:
##########
@@ -1486,5 +1487,47 @@ def __init__(self, **kwargs):
width=os.environ.get('RECORD_BREEZE_WIDTH'),
)
+
+@main.command(name='exec')
+@option_verbose
+@option_dry_run
+@click.argument('exec_args', nargs=-1, type=click.UNPROCESSED)
+def exec(
+ verbose: bool, dry_run: bool, exec_args: Tuple
+):
+ dc_run_file = BUILD_CACHE_DIR / DOCKER_COMPOSE_RUN_SCRIPT_FOR_CI
+ params = BuildCiParams()
+ ci_image_name = params.airflow_image_name
+ check_docker_resources(verbose, ci_image_name)
+ cmd = [str(dc_run_file), 'ps']
Review Comment:
Not sure entirely ;) . Docker and Docker compose filters are pretty weakly documented. But filter "give me container of the `aiflow` service" is what we are looking for. In the `base.yml` we have:
```
version: "3.7"
services:
airflow:
image: ${AIRFLOW_CI_IMAGE_WITH_TAG}
```
So our container for airflow will always run as "airflow" service.
--
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
[GitHub] [airflow] potiuk commented on pull request #23052: Breeze adding exec command
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23052:
URL: https://github.com/apache/airflow/pull/23052#issuecomment-1109793262
I just tested it an well - it works well for me :)
--
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
[GitHub] [airflow] potiuk commented on pull request #23052: Breeze adding exec command
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23052:
URL: https://github.com/apache/airflow/pull/23052#issuecomment-1108780284
I think the "exec" command has been "lost" during rebase :) - I removed all commands from the `breeze.py` so when you rebased, it has been removed during conflict resolution.
You need to re-add it :). I copied some changes from the previous version below:
```
],
},
],
"breeze exec": [
{"name": "Drops in the interactive shell of active airflow container"},
],
@main.command(name='exec', help='Joins the interactive shell of running airflow container')
@option_verbose
@option_dry_run
@click.argument('exec_args', nargs=-1, type=click.UNPROCESSED)
def exec(verbose: bool, dry_run: bool, exec_args: Tuple):
container_running = find_airflow_container(verbose, dry_run)
cmd_to_run = [
"docker",
"exec",
@@ -1510,14 +1507,17 @@ def exec(verbose: bool, dry_run: bool, exec_args: Tuple):
if exec_args:
cmd_to_run.extend(exec_args)
process = run_command(
cmd_to_run,
verbose=verbose,
dry_run=dry_run,
check=False,
no_output_dump_on_exception=False,
text=True,
)
if not process:
sys.exit(1)
sys.exit(process.returncode)
```
--
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
[GitHub] [airflow] Bowrna commented on pull request #23052: Breeze adding exec command
Posted by GitBox <gi...@apache.org>.
Bowrna commented on PR #23052:
URL: https://github.com/apache/airflow/pull/23052#issuecomment-1108769935
@potiuk Do i need to make bring any more changes to this command? Can you give your suggestion?
--
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
[GitHub] [airflow] potiuk commented on pull request #23052: Breeze adding exec command
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23052:
URL: https://github.com/apache/airflow/pull/23052#issuecomment-1112297824
Just one static check error :)
--
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
[GitHub] [airflow] potiuk commented on pull request #23052: Breeze adding exec command
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23052:
URL: https://github.com/apache/airflow/pull/23052#issuecomment-1112357081
I thin you will need to rebase after I merged #23311
--
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
[GitHub] [airflow] joppevos commented on pull request #23052: Breeze adding exec command
Posted by GitBox <gi...@apache.org>.
joppevos commented on PR #23052:
URL: https://github.com/apache/airflow/pull/23052#issuecomment-1102417805
> @potiuk Could you explain to me what will happen in this command? I tried executing `docker-compose ps` directly via terminal but it throws the following error.
`docker-compose ps` lists the containers status in a nicely formatted way. The name probably comes from the command `ps` which shows the `process status`
The error states when the command is run and it cannot find any configuration file. Check that you are running it in the right location where it can discover the config.
--
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
[GitHub] [airflow] Bowrna commented on a diff in pull request #23052: Breeze adding exec command
Posted by GitBox <gi...@apache.org>.
Bowrna commented on code in PR #23052:
URL: https://github.com/apache/airflow/pull/23052#discussion_r853082723
##########
dev/breeze/src/airflow_breeze/breeze.py:
##########
@@ -1486,5 +1487,47 @@ def __init__(self, **kwargs):
width=os.environ.get('RECORD_BREEZE_WIDTH'),
)
+
+@main.command(name='exec')
+@option_verbose
+@option_dry_run
+@click.argument('exec_args', nargs=-1, type=click.UNPROCESSED)
+def exec(
+ verbose: bool, dry_run: bool, exec_args: Tuple
+):
+ dc_run_file = BUILD_CACHE_DIR / DOCKER_COMPOSE_RUN_SCRIPT_FOR_CI
+ params = BuildCiParams()
+ ci_image_name = params.airflow_image_name
+ check_docker_resources(verbose, ci_image_name)
+ cmd = [str(dc_run_file), 'ps']
Review Comment:
I see :) I will check this. I will also take this as an opportunity to contribute to docs in Docker-compose. Thanks @potiuk
--
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
[GitHub] [airflow] Bowrna commented on pull request #23052: Breeze adding exec command
Posted by GitBox <gi...@apache.org>.
Bowrna commented on PR #23052:
URL: https://github.com/apache/airflow/pull/23052#issuecomment-1105187588
I will fix the changes that you have suggested @potiuk
I have added docker-compose filter option to get the container id
--
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
[GitHub] [airflow] Bowrna commented on pull request #23052: Breeze adding exec command
Posted by GitBox <gi...@apache.org>.
Bowrna commented on PR #23052:
URL: https://github.com/apache/airflow/pull/23052#issuecomment-1101344441
@potiuk One more question. Does the below process list only the airflow specific docker-compose containers or does it list other container too?
https://github.com/apache/airflow/blob/3a2eb961ca628d962ed5fad68760ef3439b2f40b/breeze-legacy#L3512-L3513
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #23052: Breeze adding exec command
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #23052:
URL: https://github.com/apache/airflow/pull/23052#discussion_r858755844
##########
BREEZE.rst:
##########
@@ -461,7 +461,14 @@ Those are commands mostly used by contributors:
* Initialize local virtualenv with ``./scripts/tools/initialize_virtualenv.py`` command
* Run static checks with autocomplete support ``breeze static-checks`` command
* Run test specified with ``./breeze-legacy tests`` command
-* Join running interactive shell with ``./breeze-legacy exec`` command
+* Build CI docker image with ``breeze build-image`` command
+* Cleanup breeze with ``breeze cleanup`` command
Review Comment:
I moved those commands down (to correspond with the split we have now). Also I removed the "Additional managemetn tasks" sub-split :)
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #23052: Breeze adding exec command
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #23052:
URL: https://github.com/apache/airflow/pull/23052#discussion_r858713702
##########
dev/breeze/src/airflow_breeze/commands/developer_commands.py:
##########
@@ -144,6 +144,11 @@
],
},
],
+ "breeze exec": [
Review Comment:
I thinik it should be added right after "exec" (I tried to sort it according to likelihood it's going to be used).
--
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