Skip to content

Conversation

@Bowrna
Copy link
Contributor

@Bowrna Bowrna commented Mar 3, 2022

closes : #21100
related : #21100

This helps to build prod image using Breeze2


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.

@Bowrna Bowrna force-pushed the build-prod-image branch 8 times, most recently from c203235 to 02c063a Compare March 8, 2022 08:05
@potiuk
Copy link
Member

potiuk commented Mar 8, 2022

Looks cool :)

@Bowrna Bowrna force-pushed the build-prod-image branch 12 times, most recently from 841ddf4 to 714b80c Compare March 13, 2022 13:36
@Bowrna
Copy link
Contributor Author

Bowrna commented Mar 14, 2022

@potiuk

if [[ ! ${INSTALL_AIRFLOW_VERSION} =~ ^[0-9\.]+((a|b|rc|alpha|beta|pre)[0-9]+)?$ ]]; then

Can you give me a few example airflow version that matches this pattern?

@potiuk
Copy link
Member

potiuk commented Mar 14, 2022

Can you give me a few example airflow version that matches this pattern?

  • 2.0.1alpha1
  • 2.2.2a10
  • 2.3.0rc1
  • 2.2.0.rc3
  • 2.2.3pre0

@Bowrna Bowrna force-pushed the build-prod-image branch from 714b80c to 66c3443 Compare March 14, 2022 09:01
@Bowrna
Copy link
Contributor Author

Bowrna commented Mar 14, 2022

  • 2.2.3pre

@potiuk

  • 2.0.1alpha1
  • 2.3.0rc1
    Only these patterns of version matches for me

@potiuk
Copy link
Member

potiuk commented Mar 14, 2022

Ah. right - we assume we always have number :)

@potiuk
Copy link
Member

potiuk commented Mar 14, 2022

UPDATED

@Bowrna Bowrna force-pushed the build-prod-image branch from 66c3443 to 15c1060 Compare March 15, 2022 11:12
@Bowrna
Copy link
Contributor Author

Bowrna commented Mar 15, 2022

@potiuk Could you explain to me what's happening in this code part? If GITHUB_USERNAME is not specified then will it take apache as its value?

echo "${token}" | docker_v login \
--username "${GITHUB_USERNAME:-apache}" \
--password-stdin \
"ghcr.io" || true

Copy link
Member

Choose a reason for hiding this comment

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

Nice :)

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

Just a small nit. :). If you work on tests - feel fre to add them but I am pretty much ready to merge it.

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

Just a small nit. :). If you work on tests - feel fre to add them but I am pretty much ready to merge it.

@Bowrna
Copy link
Contributor Author

Bowrna commented Mar 19, 2022

@potiuk Could you explain to me how this side effect is put to use?

check_cache_and_write_mock.side_effect = lambda cache_key, default_value: (
cache_key in cached_values,
cached_values[cache_key] if cache_key in cached_values else default_value,
)
read_from_cache_mock.side_effect = lambda param_name: cached_values.get(param_name)
build_parameters = get_image_build_params(parameters)

I could understand that you are mocking the return of those functions but I couldn't understand how it's further used in the code. If possible could you tell me or direct me to some relevant place to learn about it

@potiuk
Copy link
Member

potiuk commented Mar 20, 2022

I could understand that you are mocking the return of those functions but I couldn't understand how it's further used in the code. If possible could you tell me or direct me to some relevant place to learn about it

Sure - this is the key - lambda (in this case) is really an anonymous function:

This

 read_from_cache_mock.side_effect = lambda param_name: cached_values.get(param_name) 

Is equivalent of this (just written in a shorter way):

 def read_from_cache_mocked_function(param_name):
      return cached_values.get(param_name) 

 read_from_cache_mock.side_effect = read_from_cache_mocked_function

The "side-effect" really points to the function that will be called when the "read_from_cache_mock" funciton will be actually called. This allows to implement some logic in your test "mocks" - they become a bit smarter this way. Usually (in simple cases) your mocks can be told to return specific value:

read_from_cache_mock.return_value = x

Also you can tell it to return different values if called multiple times by assigning an iterable to return value:

read_from_cache_mock.return_value = [return_when_called_first_time, return_when_called_second_time, return_when_called_third_time]

And side_effect is the "smartest" way - you just provide your own (usually very simple) implementation of a function that can return different return values - for example based on the parameters that are passed.

In the case above, The "read_from_cache_mock" and "check_cache_and_write_mock" values are really very simple implementation tha cache the values in memory and replace the more "file-based" cache.

@Bowrna
Copy link
Contributor Author

Bowrna commented Mar 20, 2022

I see @potiuk thank you. I could better understand the code.

@Bowrna Bowrna force-pushed the build-prod-image branch 3 times, most recently from 9dc4c5e to c2e6b76 Compare March 23, 2022 10:52
@Bowrna
Copy link
Contributor Author

Bowrna commented Mar 23, 2022

@potiuk I have tried to add test for prod image similar to what you have done for CI image. Do you think I can further improve with more tests into prod image? Could you give me suggestion on this?

@Bowrna
Copy link
Contributor Author

Bowrna commented Mar 25, 2022

@potiuk Could you share your review about this PR? I made relevant changes wrt recent Dockerfile optimisation

@potiuk potiuk force-pushed the build-prod-image branch from bbb62ef to 39b0195 Compare March 27, 2022 19:19
@potiuk
Copy link
Member

potiuk commented Mar 27, 2022

Rebasing to re-run all tests :)

@Bowrna
Copy link
Contributor Author

Bowrna commented Mar 28, 2022

@potiuk Fixing the Breeze2 tests failure

@Bowrna Bowrna force-pushed the build-prod-image branch 3 times, most recently from ef5f97f to d4b7743 Compare March 29, 2022 04:02
@Bowrna Bowrna force-pushed the build-prod-image branch from d4b7743 to 4da1c05 Compare March 29, 2022 04:03
@potiuk potiuk merged commit 4eebabb into apache:main Mar 29, 2022
@potiuk
Copy link
Member

potiuk commented Mar 29, 2022

Wooho! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:dev-tools okay to merge It's ok to merge this PR as it does not require more tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Breeze: Build PROD image with Breeze

2 participants