Skip to content

[AIRFLOW-5830] Get rid of slim image.#6494

Merged
potiuk merged 1 commit into
apache:masterfrom
PolideaInternal:get-rid-of-slim-image
Nov 5, 2019
Merged

[AIRFLOW-5830] Get rid of slim image.#6494
potiuk merged 1 commit into
apache:masterfrom
PolideaInternal:get-rid-of-slim-image

Conversation

@potiuk
Copy link
Copy Markdown
Member

@potiuk potiuk commented Nov 4, 2019 •
edited
Loading

The slim image gave only very small gain on executing the tests in CI. The
image was significantly smaller, but then for local development and testing
you needed both full CI and SLIM-CI image.

This made the scripts and docker image needlessly complex - especially
in the wake of coming Production image it turned to be premature
optimisation really. While it sped-up (slightly - by 10-20 seconds) some
static check jobs in Travis, it increased time needed by developers
to have a working environment and to keep it updated every time it was
needed (by minutes)

Also having two separately managed images made it rather complex to join
some of the Travis CI jobs (there is a follow-up change with getting rid
of Checklicence image).

With this change both static checks and tests are executed using single
image. That also opens doors for further simplification of the scripts
and easier implementation of production image.

Jira

Description

  • Here are some details about my PR, including screenshots of any UI changes:

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

  • My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain docstrings that explain what it does
    • If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

@potiuk potiuk self-assigned this Nov 4, 2019
This was referenced Nov 4, 2019
Merged
Merged
@potiuk potiuk force-pushed the get-rid-of-slim-image branch from 7267db7 to 60022f4 Compare November 4, 2019 09:50
@potiuk potiuk changed the title [AIRFLOW-5830] Get rid of slim image (depends on [AIRFLOW-5826] [AIRFLOW-5827]) [AIRFLOW-5830] Get rid of slim image. Depends on [AIRFLOW-5827] Nov 4, 2019
@codecov-io
Copy link
Copy Markdown

codecov-io commented Nov 4, 2019 •
edited
Loading

Codecov Report

Merging #6494 into master will decrease coverage by 0.59%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #6494     +/-   ##
=========================================
- Coverage   84.01%   83.41%   -0.6%     
=========================================
  Files         635      635             
  Lines       36678    36678             
=========================================
- Hits        30815    30595    -220     
- Misses       5863     6083    +220
Impacted Files Coverage Δ
airflow/operators/mysql_operator.py 0% <0%> (-100%) ⬇️
airflow/operators/mysql_to_hive.py 0% <0%> (-100%) ⬇️
airflow/kubernetes/volume_mount.py 44.44% <0%> (-55.56%) ⬇️
airflow/kubernetes/volume.py 52.94% <0%> (-47.06%) ⬇️
airflow/kubernetes/pod_launcher.py 45.25% <0%> (-46.72%) ⬇️
airflow/kubernetes/kube_client.py 33.33% <0%> (-41.67%) ⬇️
...rflow/contrib/operators/kubernetes_pod_operator.py 70.14% <0%> (-28.36%) ⬇️
airflow/utils/sqlalchemy.py 86.44% <0%> (-6.78%) ⬇️
airflow/jobs/local_task_job.py 85% <0%> (-5%) ⬇️
airflow/hooks/hive_hooks.py 75.82% <0%> (-1.79%) ⬇️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 45e108d...39f13ea. Read the comment docs.

mik-laj
mik-laj reviewed Nov 4, 2019
View reviewed changes
Comment thread BREEZE.rst Outdated
mik-laj
mik-laj reviewed Nov 4, 2019
View reviewed changes
Comment thread scripts/ci/_utils.sh Outdated
@@ -239,8 +239,6 @@ function update_all_md5_files() {
# files. There are the following, problems with the current Dockerfiles that need longer build times:
# 1) We need to fix group permissions of files in Docker because different linux build services have
Copy link
Copy Markdown
Member

@mik-laj mik-laj Nov 4, 2019

Choose a reason for hiding this comment

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

This numbered list does not make sense now. The previous paragraph talks about many problems, and only one problem is described.

Copy link
Copy Markdown
Member Author

@potiuk potiuk Nov 4, 2019

Choose a reason for hiding this comment

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

True!

mik-laj
mik-laj reviewed Nov 4, 2019
View reviewed changes
Comment thread Dockerfile Outdated
mik-laj
mik-laj reviewed Nov 4, 2019
View reviewed changes
Comment thread Dockerfile Outdated
mik-laj
mik-laj reviewed Nov 4, 2019
View reviewed changes
Comment thread hooks/build
mik-laj
mik-laj reviewed Nov 4, 2019
View reviewed changes
Comment thread scripts/ci/_utils.sh Outdated
mik-laj
mik-laj approved these changes Nov 4, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@mik-laj mik-laj left a comment

Choose a reason for hiding this comment

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

It looks good, but I think another person should look to be sure.

Fokko
Fokko approved these changes Nov 4, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

Love it @potiuk.

@potiuk potiuk force-pushed the get-rid-of-slim-image branch from 60022f4 to 850d5d7 Compare November 4, 2019 19:20
@potiuk potiuk changed the title [AIRFLOW-5830] Get rid of slim image. Depends on [AIRFLOW-5827] [AIRFLOW-5830] Get rid of slim image. Nov 4, 2019
@potiuk
[AIRFLOW-5830] Get rid of slim image
39f13ea 
The slim image gave only very small gain on executing the tests in CI. The
image was significantly smaller, but then for local development and testing
you needed both full CI and SLIM-CI image.

This made the scripts and docker image needlessly complex - especially
in the wake of coming Production image it turned to be premature
optimisation really. While it sped-up (slightly - by 10-20 seconds) some
static check jobs in Travis, it increased time needed by developers
to have a working environment and to keep it updated every time it was
needed (by minutes)

Also having two separately managed images made it rather complex to join
some of the Travis CI jobs (there is a follow-up change with getting rid
of Checklicence image).

With this change both static checks and tests are executed using single
image. That also opens doors for further simplification of the scripts
and easier implementation of production image.
@potiuk potiuk force-pushed the get-rid-of-slim-image branch from 850d5d7 to 39f13ea Compare November 4, 2019 20:44
This was referenced Nov 4, 2019
Closed
Closed
@potiuk potiuk merged commit b4c7538 into apache:master Nov 5, 2019
potiuk added a commit that referenced this pull request Nov 6, 2019
@potiuk
[AIRFLOW-5830] Get rid of slim image (#6494)
9ae2ffd 
The slim image gave only very small gain on executing the tests in CI. The
image was significantly smaller, but then for local development and testing
you needed both full CI and SLIM-CI image.

This made the scripts and docker image needlessly complex - especially
in the wake of coming Production image it turned to be premature
optimisation really. While it sped-up (slightly - by 10-20 seconds) some
static check jobs in Travis, it increased time needed by developers
to have a working environment and to keep it updated every time it was
needed (by minutes)

Also having two separately managed images made it rather complex to join
some of the Travis CI jobs (there is a follow-up change with getting rid
of Checklicence image).

With this change both static checks and tests are executed using single
image. That also opens doors for further simplification of the scripts
and easier implementation of production image.

(cherry picked from commit b4c7538)
potiuk added a commit that referenced this pull request Nov 6, 2019
@potiuk
[AIRFLOW-5830] Get rid of slim image (#6494)
0bfc20a 
The slim image gave only very small gain on executing the tests in CI. The
image was significantly smaller, but then for local development and testing
you needed both full CI and SLIM-CI image.

This made the scripts and docker image needlessly complex - especially
in the wake of coming Production image it turned to be premature
optimisation really. While it sped-up (slightly - by 10-20 seconds) some
static check jobs in Travis, it increased time needed by developers
to have a working environment and to keep it updated every time it was
needed (by minutes)

Also having two separately managed images made it rather complex to join
some of the Travis CI jobs (there is a follow-up change with getting rid
of Checklicence image).

With this change both static checks and tests are executed using single
image. That also opens doors for further simplification of the scripts
and easier implementation of production image.

(cherry picked from commit b4c7538)
potiuk added a commit that referenced this pull request Nov 12, 2019
@potiuk
[AIRFLOW-5830] Get rid of slim image (#6494)
5443764 
The slim image gave only very small gain on executing the tests in CI. The
image was significantly smaller, but then for local development and testing
you needed both full CI and SLIM-CI image.

This made the scripts and docker image needlessly complex - especially
in the wake of coming Production image it turned to be premature
optimisation really. While it sped-up (slightly - by 10-20 seconds) some
static check jobs in Travis, it increased time needed by developers
to have a working environment and to keep it updated every time it was
needed (by minutes)

Also having two separately managed images made it rather complex to join
some of the Travis CI jobs (there is a follow-up change with getting rid
of Checklicence image).

With this change both static checks and tests are executed using single
image. That also opens doors for further simplification of the scripts
and easier implementation of production image.

(cherry picked from commit b4c7538)
eladkal pushed a commit to eladkal/airflow that referenced this pull request Dec 2, 2019
@potiuk @eladkal
[AIRFLOW-5830] Get rid of slim image (apache#6494)
d749888 
The slim image gave only very small gain on executing the tests in CI. The
image was significantly smaller, but then for local development and testing
you needed both full CI and SLIM-CI image.

This made the scripts and docker image needlessly complex - especially
in the wake of coming Production image it turned to be premature
optimisation really. While it sped-up (slightly - by 10-20 seconds) some
static check jobs in Travis, it increased time needed by developers
to have a working environment and to keep it updated every time it was
needed (by minutes)

Also having two separately managed images made it rather complex to join
some of the Travis CI jobs (there is a follow-up change with getting rid
of Checklicence image).

With this change both static checks and tests are executed using single
image. That also opens doors for further simplification of the scripts
and easier implementation of production image.

(cherry picked from commit b4c7538)
kaxil pushed a commit that referenced this pull request Dec 12, 2019
@potiuk @kaxil
[AIRFLOW-5830] Get rid of slim image (#6494)
b15062e 
The slim image gave only very small gain on executing the tests in CI. The
image was significantly smaller, but then for local development and testing
you needed both full CI and SLIM-CI image.

This made the scripts and docker image needlessly complex - especially
in the wake of coming Production image it turned to be premature
optimisation really. While it sped-up (slightly - by 10-20 seconds) some
static check jobs in Travis, it increased time needed by developers
to have a working environment and to keep it updated every time it was
needed (by minutes)

Also having two separately managed images made it rather complex to join
some of the Travis CI jobs (there is a follow-up change with getting rid
of Checklicence image).

With this change both static checks and tests are executed using single
image. That also opens doors for further simplification of the scripts
and easier implementation of production image.

(cherry picked from commit b4c7538)
@potiuk potiuk added the area:production-image Production image improvements and fixes label Apr 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Fokko Fokko Fokko approved these changes

@mik-laj mik-laj mik-laj approved these changes

@ashb ashb Awaiting requested review from ashb

@dimberman dimberman Awaiting requested review from dimberman

Assignees

@potiuk potiuk

Labels

area:production-image Production image improvements and fixes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@potiuk @codecov-io @Fokko @mik-laj