Skip to content

Conversation

@potiuk
Copy link
Member

@potiuk potiuk commented Dec 8, 2023

So far we executed commands in CI image in breeze in two ways:

  • entering the shell (which runs docker-compose under the hood)
  • running docker run with the CI image

This requires rather complex mapping of environment variables
between docker-compose and docker. Since recently (#35862)
we can use shell command to run commands in very similar way
as docker run (with docker-compose, without database and extra
components - just using the same breeze shell mechanisms.

This PR converts all the usages of docker run CI_IMAGE we had
and converts them to use modified enter_shell method that has
been moved to "docker_command_utils". This also simplified
passing arguments to the "enter_shell" command - no longer need
to filter out none parameters and **kwargs - all parameters are
passed explicitly.

This also allowed to remove some of the code (extracting args,
filtering_out_none) that are not used anymore.

The entypoint CI has been slightly refactored - to provide
a bit better structure and handle --skip-environment-initialization
better - we can now both set --use-airflow-version and
--skip-environment-initialization which was not possible
before.


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@potiuk potiuk force-pushed the use-breeze-cmd-run-for-release-commands branch 4 times, most recently from e17136a to 874cee9 Compare December 9, 2023 09:56
@potiuk potiuk marked this pull request as ready for review December 9, 2023 09:56
@potiuk potiuk requested review from Taragolis, dstandish, eladkal, utkarsharma2 and vincbeck and removed request for ashb and jedcunningham December 9, 2023 09:57
Copy link
Member Author

@potiuk potiuk Dec 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one will not be needed anymore

@potiuk potiuk force-pushed the use-breeze-cmd-run-for-release-commands branch 10 times, most recently from 3353a58 to 3ca4264 Compare December 10, 2023 12:21
@potiuk
Copy link
Member Author

potiuk commented Dec 10, 2023

Right. Solved all the problems - should be ready to review now :).

This is one more cleanup (including removal of +200 lines of redundant codee) where we have an easier way to understand and add new commands for breeze in the future.

@potiuk potiuk force-pushed the use-breeze-cmd-run-for-release-commands branch from 3ca4264 to b574a28 Compare December 10, 2023 13:34
So far we executed commands in CI image in breeze in two ways:

* entering the shell (which runs docker-compose under the hood)
* running `docker run` with the CI image

This requires rather complex mapping of environment variables
between `docker-compose` and `docker`. Since recently (apache#35862)
we can use `shell` command to run commands in very similar way
as docker run (with docker-compose, without database and extra
components - just using the same `breeze shell` mechanisms.

This PR converts all the usages of docker run CI_IMAGE we had
and converts them to use modified `enter_shell` method that has
been moved to "docker_command_utils". This also simplified
passing arguments to the "enter_shell" command - no longer need
to filter out none parameters and **kwargs - all parameters are
passed explicitly.

This also allowed to remove some of the code (extracting args,
filtering_out_none) that are not used anymore.

The entypoint CI has been slightly refactored - to provide
a bit better structure and handle `--skip-environment-initialization`
better - we can now both set `--use-airflow-version` and
`--skip-environment-initialization` which was not possible
before.
@potiuk potiuk force-pushed the use-breeze-cmd-run-for-release-commands branch from b574a28 to ad1d77b Compare December 10, 2023 14:01
@potiuk potiuk merged commit 2d15dbf into apache:main Dec 10, 2023
@potiuk potiuk deleted the use-breeze-cmd-run-for-release-commands branch December 10, 2023 15:07
potiuk added a commit to potiuk/airflow that referenced this pull request Dec 10, 2023
When we are testing packages in main, we also test sdist packages,
and main is the only build we run it, so when implementing apache#36131
we had no chance to test it. There was an etra USE_AIRFLOW_VERSION
variable set in the job and it caused failed attempt of reinstalling
sdist package when entering constraints generation.

This variable is now removed, only the --use-airflow-version flags
in breeze commands that need it are used.
potiuk added a commit that referenced this pull request Dec 10, 2023
…36154)

When we are testing packages in main, we also test sdist packages,
and main is the only build we run it, so when implementing #36131
we had no chance to test it. There was an etra USE_AIRFLOW_VERSION
variable set in the job and it caused failed attempt of reinstalling
sdist package when entering constraints generation.

This variable is now removed, only the --use-airflow-version flags
in breeze commands that need it are used.
potiuk added a commit to potiuk/airflow that referenced this pull request Dec 10, 2023
The change apache#36131 when switching to docker-compose based running of
scripts introduced a subtle race condition when several docker-compose
runs were running in paralell and used the same project_name. This
happened for example in case of constraints generation - when
running two Python constraints generation caused docker network to
be created two times.

This PR fixes the problem for the cases where such run are run in
parallel - by giving each of the parallel commands unique project_name.
potiuk added a commit that referenced this pull request Dec 10, 2023
…#36155)

The change #36131 when switching to docker-compose based running of
scripts introduced a subtle race condition when several docker-compose
runs were running in paralell and used the same project_name. This
happened for example in case of constraints generation - when
running two Python constraints generation caused docker network to
be created two times.

This PR fixes the problem for the cases where such run are run in
parallel - by giving each of the parallel commands unique project_name.
potiuk added a commit to potiuk/airflow that referenced this pull request Dec 11, 2023
The change apache#36131 made a subtle change to the behaviour of
tests - they used to have credentials forwarded by default, but
the intention was that that the credentials are only forwarded
via explicit flag passed by the user (due to security).

This change adds `--forward-credentials` to all test commands.
potiuk added a commit that referenced this pull request Dec 11, 2023
The change #36131 made a subtle change to the behaviour of
tests - they used to have credentials forwarded by default, but
the intention was that that the credentials are only forwarded
via explicit flag passed by the user (due to security).

This change adds `--forward-credentials` to all test commands.
potiuk added a commit that referenced this pull request Dec 15, 2023
So far we executed commands in CI image in breeze in two ways:

* entering the shell (which runs docker-compose under the hood)
* running `docker run` with the CI image

This requires rather complex mapping of environment variables
between `docker-compose` and `docker`. Since recently (#35862)
we can use `shell` command to run commands in very similar way
as docker run (with docker-compose, without database and extra
components - just using the same `breeze shell` mechanisms.

This PR converts all the usages of docker run CI_IMAGE we had
and converts them to use modified `enter_shell` method that has
been moved to "docker_command_utils". This also simplified
passing arguments to the "enter_shell" command - no longer need
to filter out none parameters and **kwargs - all parameters are
passed explicitly.

This also allowed to remove some of the code (extracting args,
filtering_out_none) that are not used anymore.

The entypoint CI has been slightly refactored - to provide
a bit better structure and handle `--skip-environment-initialization`
better - we can now both set `--use-airflow-version` and
`--skip-environment-initialization` which was not possible
before.

(cherry picked from commit 2d15dbf)
potiuk added a commit that referenced this pull request Dec 15, 2023
…#36155)

The change #36131 when switching to docker-compose based running of
scripts introduced a subtle race condition when several docker-compose
runs were running in paralell and used the same project_name. This
happened for example in case of constraints generation - when
running two Python constraints generation caused docker network to
be created two times.

This PR fixes the problem for the cases where such run are run in
parallel - by giving each of the parallel commands unique project_name.

(cherry picked from commit 643b234)
potiuk added a commit that referenced this pull request Dec 15, 2023
The change #36131 made a subtle change to the behaviour of
tests - they used to have credentials forwarded by default, but
the intention was that that the credentials are only forwarded
via explicit flag passed by the user (due to security).

This change adds `--forward-credentials` to all test commands.

(cherry picked from commit f133643)
@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:dev-tools changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants