Skip to content

Conversation

@shubhparekh
Copy link
Contributor

@shubhparekh shubhparekh commented Jan 5, 2019

Fixed zipped Dag trigger for Web and Api

Make sure you have checked all steps below.

Jira

Description

  • Conversion from Dagbag() to Dagbag(fileloc_of_zip) fails.
  • This happened with the following commits for api and for webserver
  • The above commits are necessary to speed up trigger of dag else it will parse all dags again.
  • Hence added utility function similar to open_maybe_zipped to handle cases where fileloc can be a zipped python file. e.g. ~/airflow/dags/example.zip/example.py

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.
    • When adding new operators/hooks/sensors, the autoclass documentation generation needs to be added.
    • All the public functions and the classes in the PR contain docstrings that explain what it does

Code Quality

  • Passes flake8

@shubhparekh
Copy link
Contributor Author

shubhparekh commented Jan 5, 2019

Can someone help to fix this fail previously the same code ran successfully on travis here. Getting strange behaviour.

@shubhparekh shubhparekh closed this Jan 5, 2019
@shubhparekh shubhparekh reopened this Jan 5, 2019
@shubhparekh shubhparekh closed this Jan 5, 2019
@shubhparekh shubhparekh reopened this Jan 5, 2019
@codecov-io
Copy link

codecov-io commented Jan 5, 2019

Codecov Report

Merging #4439 into master will decrease coverage by 0.77%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4439      +/-   ##
==========================================
- Coverage   77.01%   76.23%   -0.78%     
==========================================
  Files         463      466       +3     
  Lines       29753    30104     +351     
==========================================
+ Hits        22915    22951      +36     
- Misses       6838     7153     +315
Impacted Files Coverage Δ
airflow/models/__init__.py 93.01% <100%> (-6.99%) ⬇️
airflow/utils/dag_processing.py 59.58% <80%> (+0.17%) ⬆️
airflow/contrib/hooks/cloudant_hook.py 0% <0%> (-100%) ⬇️
airflow/contrib/hooks/slack_webhook_hook.py 85.71% <0%> (-9.53%) ⬇️
airflow/operators/check_operator.py 91.79% <0%> (-0.86%) ⬇️
airflow/ti_deps/deps/trigger_rule_dep.py 90.14% <0%> (-0.77%) ⬇️
airflow/contrib/operators/gcs_download_operator.py 88% <0%> (-0.47%) ⬇️
airflow/models/xcom.py 79.79% <0%> (-0.41%) ⬇️
airflow/utils/helpers.py 82.51% <0%> (-0.36%) ⬇️
... and 54 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 7d3d740...4ffffdc. Read the comment docs.

@shubhparekh shubhparekh changed the title [AIRFLOW-3626] Fixed zipped Dag trigger [AIRFLOW-3626] Fix zipped Dag trigger Jan 11, 2019
@shubhparekh shubhparekh changed the title [AIRFLOW-3626] Fix zipped Dag trigger [AIRFLOW-3626] Fixed zipped Dag trigger Jan 11, 2019
@shubhparekh shubhparekh reopened this Jan 20, 2019
@ashb
Copy link
Member

ashb commented Jan 21, 2019

This needs tests please so it doesn't break again. There is a tests/dags/test_zip.zip already in the repo that contains a dag or two that you can use in the tests.

@shubhparekh
Copy link
Contributor Author

This needs tests please so it doesn't break again. There is a tests/dags/test_zip.zip already in the repo that contains a dag or two that you can use in the tests.

I will have a look.

@shubhparekh
Copy link
Contributor Author

@ashb I tested for the zipped test dag you mentioned, I am able to trigger the dag successfully.

@kurtqq
Copy link
Contributor

kurtqq commented Mar 4, 2019

@shubhparekh I think he means that you need to add a test to https://github.com/apache/airflow/blob/master/tests/utils/test_dag_processing.py that verify that you code change behave as expected and make sure that future code changes won't break it. For that aim you can use the tests/dags/test_zip.zip in the test function.

@shubhparekh
Copy link
Contributor Author

@shubhparekh I think he means that you need to add a test to https://github.com/apache/airflow/blob/master/tests/utils/test_dag_processing.py that verify that you code change behave as expected and make sure that future code changes won't break it. For that aim you can use the tests/dags/test_zip.zip in the test function.

I got it now I will do the changes.

@OmerJog
Copy link
Contributor

OmerJog commented Apr 11, 2019

@shubhparekh can you rebase?

@kurtqq
Copy link
Contributor

kurtqq commented Apr 16, 2019

If you wrote tests then you forgot to commit them to the PR

@shubhparekh
Copy link
Contributor Author

@kurtqq I have just started learning about writing unit tests. The tests that are already written seem quite complex to me. So have not written one.

Fixed zipped Dag trigger for Web and Api
@shubhparekh
Copy link
Contributor Author

@kurtqq Added unittest please review.

@ashb ashb removed the needs-fixes label Apr 29, 2019
@ashb ashb merged commit e518a1e into apache:master Apr 29, 2019
ashb pushed a commit that referenced this pull request Apr 29, 2019
ashb pushed a commit to ashb/airflow that referenced this pull request Apr 29, 2019
andriisoldatenko pushed a commit to andriisoldatenko/airflow that referenced this pull request Jul 26, 2019
wmorris75 pushed a commit to modmed-external/incubator-airflow that referenced this pull request Jul 29, 2019
dharamsk pushed a commit to postmates/airflow that referenced this pull request Aug 8, 2019
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Jan 16, 2020
* changes:
  Cherrypick [AIRFLOW-4760] PR apache/airflow#5404
  Cherrypick [AIRFLOW-3626] apache/airflow#4439

GitOrigin-RevId: 05ea000a888d92c30b83c649efc888cbeb149a6e
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Jan 23, 2020
* changes:
  Cherrypick [AIRFLOW-4760] PR apache/airflow#5404
  Cherrypick [AIRFLOW-3626] apache/airflow#4439

GitOrigin-RevId: 05ea000a888d92c30b83c649efc888cbeb149a6e
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Apr 8, 2020
* changes:
  Cherrypick [AIRFLOW-4760] PR apache/airflow#5404
  Cherrypick [AIRFLOW-3626] apache/airflow#4439

GitOrigin-RevId: 05ea000a888d92c30b83c649efc888cbeb149a6e
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Jun 15, 2021
* changes:
  Cherrypick [AIRFLOW-4760] PR apache/airflow#5404
  Cherrypick [AIRFLOW-3626] apache/airflow#4439

GitOrigin-RevId: 05ea000a888d92c30b83c649efc888cbeb149a6e
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.

5 participants