Skip to content

[AIRFLOW-3925] Don't pull docker-images on pretest#4740

Merged
ashb merged 1 commit into
apache:masterfrom
mik-laj:AIRFLOW-3925
Feb 22, 2019
Merged

[AIRFLOW-3925] Don't pull docker-images on pretest#4740
ashb merged 1 commit into
apache:masterfrom
mik-laj:AIRFLOW-3925

Conversation

@mik-laj

@mik-laj mik-laj commented Feb 20, 2019

Copy link
Copy Markdown
Member

Make sure you have checked all steps below.

Jira

Description

We do not need a docker in pretest, so we should not pull images.
The problem has previously been discussed:
#4685 (comment)

Tests

No applicable

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

No applicable

Code Quality

No applicable

Co-authored-by: Joshua Carp <jm.carp@gmail.com>
@codecov-io

Copy link
Copy Markdown

Codecov Report

Merging #4740 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #4740      +/-   ##
=========================================
- Coverage   74.61%   74.6%   -0.01%     
=========================================
  Files         430     430              
  Lines       28002   28002              
=========================================
- Hits        20893   20892       -1     
- Misses       7109    7110       +1
Impacted Files Coverage Δ
airflow/models/__init__.py 92.59% <0%> (-0.06%) ⬇️

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 6bfa0ba...16ef81b. Read the comment docs.

@codecov-io

codecov-io commented Feb 20, 2019

Copy link
Copy Markdown

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4740      +/-   ##
==========================================
- Coverage   74.61%   74.09%   -0.52%     
==========================================
  Files         430      431       +1     
  Lines       28002    28214     +212     
==========================================
+ Hits        20893    20906      +13     
- Misses       7109     7308     +199
Impacted Files Coverage Δ
airflow/sensors/sql_sensor.py 57.57% <0%> (-42.43%) ⬇️
airflow/www/api/experimental/endpoints.py 60.74% <0%> (-27.44%) ⬇️
airflow/utils/dag_processing.py 53.12% <0%> (-6.78%) ⬇️
airflow/api/common/experimental/get_code.py 0% <0%> (ø)

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 6bfa0ba...16ef81b. Read the comment docs.

@jmcarp

jmcarp commented Feb 20, 2019

Copy link
Copy Markdown
Contributor

This is already included in #4685.

@mik-laj

mik-laj commented Feb 20, 2019

Copy link
Copy Markdown
Member Author

I know about it. the purpose of your PR is not to fix the problem presented in this PR, but it does it additionally. Your PR is large and have discussion. I would like this change to come faster, so I made a separate PR.
I added you as a co-author of commit.

@drewsonne

Copy link
Copy Markdown
Contributor

Also in here #4703 :-)

@jmcarp

jmcarp commented Feb 22, 2019

Copy link
Copy Markdown
Contributor

OK, if #4685 and #4703 will take more time to review, let's merge this and start saving time on pre-test steps now.

@ashb

ashb commented Feb 22, 2019

Copy link
Copy Markdown
Member

+1 to the change, but for those of us who aren't familiar with the travis config file (me) could someone explain/point me at docs as to how this stops it pulling a docker image on pre-test?

@drewsonne

drewsonne commented Feb 22, 2019

Copy link
Copy Markdown
Contributor

@ashb by default, Travis has the job lifecycles for a single implicit stage ("Test").

When you add the jobs directive, you create new stages, and the jobs in those stages inherit from the default implicit stage ("Test"). So the main part here, is that the jobs define their own actions in the install lifecycle event, which override the inherited install from the default stage ("Test").

I'm not sure if it's made clear on these pages, but this should be the docs for this:

https://docs.travis-ci.com/user/build-stages/#how-do-build-stages-work
https://docs.travis-ci.com/user/job-lifecycle/

@Fokko

Fokko commented Feb 22, 2019

Copy link
Copy Markdown
Contributor

Looks like a great improvements.

Before:
image

After:
image

@ashb ashb merged commit 240138f into apache:master Feb 22, 2019
antonimaciej pushed a commit to PolideaInternal/airflow that referenced this pull request Feb 26, 2019
Co-authored-by: Joshua Carp <jm.carp@gmail.com>
ashb pushed a commit to ashb/airflow that referenced this pull request Mar 27, 2019
Co-authored-by: Joshua Carp <jm.carp@gmail.com>
wmorris75 pushed a commit to modmed-external/incubator-airflow that referenced this pull request Jul 29, 2019
Co-authored-by: Joshua Carp <jm.carp@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants