Make passing build args explicit in ci/prod builds #35768

potiuk · 2023-11-21T02:06:54Z

When building hte image, breeze converts some simple parameters passed as breeze command (with autocompletion and explanation) into much longer and more complex set of build args that are passed to docker build command. The way how passing hte args worked so far is that it was pretty implicit:

**kwargs were used to ingest click flags
parameters found as empty/None were filtered out from these
Build*Params dataclass was created out of such kwargs dict
argumenst from dataclass (with some customization) were converted to --build-arg (CAPITALIZED_PROPERTY_NAME)

This had a lot of implicitness and it was not easy to understand whether the parameters passed were correct and how they passed through this chain.

This change makes all the build arg much more explicit - without kwargs and dictionary. Each CI/PROD build param has now a method where it explicitly converts arguments into build-args - including specifying which of those are optional (where you can actually filter out Empty and None values) and which are required (where an actual value is expected).

^ 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 · 2023-11-21T02:13:34Z

dev/breeze/src/airflow_breeze/commands/ci_image_commands.py

@@ -550,13 +614,6 @@ def run_build_ci_image(
    :param ci_image_params: CI image parameters
    :param output: output redirection
    """
-    if not ci_image_params.version_suffix_for_pypi:


Moved as default value for CI build

potiuk · 2023-11-21T02:14:02Z

dev/breeze/src/airflow_breeze/commands/ci_image_commands_config.py

@@ -35,7 +35,7 @@
                "--image-tag",
                "--tag-as-latest",
                "--docker-cache",
-                "--force-build",


It was not used. When you order a build build is forced.

When building hte image, breeze converts some simple parameters passed as breeze command (with autocompletion and explanation) into much longer and more complex set of build args that are passed to `docker build` command. The way how passing hte args worked so far is that it was pretty implicit: * **kwargs were used to ingest `click` flags * parameters found as empty/None were filtered out from these * Build*Params dataclass was created out of such kwargs dict * argumenst from dataclass (with some customization) were converted to --build-arg (CAPITALIZED_PROPERTY_NAME) This had a lot of implicitness and it was not easy to understand whether the parameters passed were correct and how they passed through this chain. This change makes all the build arg much more explicit - without kwargs and dictionary. Each CI/PROD build param has now a method where it explicitly converts arguments into build-args - including specifying which of those are optional (where you can actually filter out Empty and None values) and which are required (where an actual value is expected). This PR also cleans up the click flags sequence and their presence as well as the output of help command (they were grouped with more related parameters)

potiuk · 2023-11-21T10:07:37Z

cc: @Bowrna -> rewritten the old breeze argument passing now to be more explicit :)

potiuk · 2023-11-21T10:10:14Z

Result of discussion in https://apache-airflow.slack.com/archives/CQ9QHSFQX/p1700490311364209

potiuk · 2023-11-21T11:38:40Z

I checked carefully and mapping of parmeters to build args looks good - I also cleaned up some duplicates along the way (like duplicate --progress`

The `--force-build` command has not been used for a long time in the `ci-image build` command for quite some time because the sheer fact that you run the command means that you want to run the build (so it made no sense to have it). The command only makes sense when you want to force build when running `breeze` or `breeze start-airflow` command. However commands to build cache in CI still had this command used and it caused the builds to fail after merging apache#35768 when the option has been removed (it was not detected there, because cache steps only run in main build)

The `--force-build` command has not been used for a long time in the `ci-image build` command for quite some time because the sheer fact that you run the command means that you want to run the build (so it made no sense to have it). The command only makes sense when you want to force build when running `breeze` or `breeze start-airflow` command. However commands to build cache in CI still had this command used and it caused the builds to fail after merging #35768 when the option has been removed (it was not detected there, because cache steps only run in main build)

When building hte image, breeze converts some simple parameters passed as breeze command (with autocompletion and explanation) into much longer and more complex set of build args that are passed to `docker build` command. The way how passing hte args worked so far is that it was pretty implicit: * **kwargs were used to ingest `click` flags * parameters found as empty/None were filtered out from these * Build*Params dataclass was created out of such kwargs dict * argumenst from dataclass (with some customization) were converted to --build-arg (CAPITALIZED_PROPERTY_NAME) This had a lot of implicitness and it was not easy to understand whether the parameters passed were correct and how they passed through this chain. This change makes all the build arg much more explicit - without kwargs and dictionary. Each CI/PROD build param has now a method where it explicitly converts arguments into build-args - including specifying which of those are optional (where you can actually filter out Empty and None values) and which are required (where an actual value is expected). This PR also cleans up the click flags sequence and their presence as well as the output of help command (they were grouped with more related parameters)

The `--force-build` command has not been used for a long time in the `ci-image build` command for quite some time because the sheer fact that you run the command means that you want to run the build (so it made no sense to have it). The command only makes sense when you want to force build when running `breeze` or `breeze start-airflow` command. However commands to build cache in CI still had this command used and it caused the builds to fail after merging #35768 when the option has been removed (it was not detected there, because cache steps only run in main build)

When building hte image, breeze converts some simple parameters passed as breeze command (with autocompletion and explanation) into much longer and more complex set of build args that are passed to `docker build` command. The way how passing hte args worked so far is that it was pretty implicit: * **kwargs were used to ingest `click` flags * parameters found as empty/None were filtered out from these * Build*Params dataclass was created out of such kwargs dict * argumenst from dataclass (with some customization) were converted to --build-arg (CAPITALIZED_PROPERTY_NAME) This had a lot of implicitness and it was not easy to understand whether the parameters passed were correct and how they passed through this chain. This change makes all the build arg much more explicit - without kwargs and dictionary. Each CI/PROD build param has now a method where it explicitly converts arguments into build-args - including specifying which of those are optional (where you can actually filter out Empty and None values) and which are required (where an actual value is expected). This PR also cleans up the click flags sequence and their presence as well as the output of help command (they were grouped with more related parameters)

The `--force-build` command has not been used for a long time in the `ci-image build` command for quite some time because the sheer fact that you run the command means that you want to run the build (so it made no sense to have it). The command only makes sense when you want to force build when running `breeze` or `breeze start-airflow` command. However commands to build cache in CI still had this command used and it caused the builds to fail after merging #35768 when the option has been removed (it was not detected there, because cache steps only run in main build)

boring-cyborg bot added the area:dev-tools label Nov 21, 2023

potiuk force-pushed the explicit_build_params branch from b92c02b to 29aa280 Compare November 21, 2023 02:13

potiuk commented Nov 21, 2023

View reviewed changes

potiuk force-pushed the explicit_build_params branch from 29aa280 to c391940 Compare November 21, 2023 02:26

potiuk requested a review from vincbeck November 21, 2023 02:26

potiuk force-pushed the explicit_build_params branch from c391940 to c350c37 Compare November 21, 2023 02:43

potiuk marked this pull request as ready for review November 21, 2023 10:06

potiuk requested review from ashb and jedcunningham as code owners November 21, 2023 10:06

potiuk requested review from kaxil, hussein-awala and amoghrajesh November 21, 2023 10:07

potiuk requested review from ephraimbuddy, pankajkoti, eladkal and pankajastro November 21, 2023 10:07

potiuk force-pushed the explicit_build_params branch from c350c37 to 72ab626 Compare November 21, 2023 10:08

vincbeck approved these changes Nov 21, 2023

View reviewed changes

potiuk merged commit 50e0b92 into main Nov 21, 2023
72 checks passed

potiuk mentioned this pull request Nov 21, 2023

Remove --force-build command in cache steps in CI #35784

Merged

potiuk mentioned this pull request Nov 22, 2023

Consolidate environment variable calculation in ShellParams in Breeze #35801

Merged

Taragolis deleted the explicit_build_params branch December 27, 2023 10:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make passing build args explicit in ci/prod builds #35768

Make passing build args explicit in ci/prod builds #35768

potiuk commented Nov 21, 2023

potiuk Nov 21, 2023

potiuk Nov 21, 2023

potiuk commented Nov 21, 2023

potiuk commented Nov 21, 2023

potiuk commented Nov 21, 2023

Make passing build args explicit in ci/prod builds #35768

Make passing build args explicit in ci/prod builds #35768

Conversation

potiuk commented Nov 21, 2023

potiuk Nov 21, 2023

Choose a reason for hiding this comment

potiuk Nov 21, 2023

Choose a reason for hiding this comment

potiuk commented Nov 21, 2023

potiuk commented Nov 21, 2023

potiuk commented Nov 21, 2023