Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[AIRFLOW-4746] Implement GCP Cloud Tasks' Hook and Operators #5402

Merged
merged 1 commit into from Aug 7, 2019
Merged

[AIRFLOW-4746] Implement GCP Cloud Tasks' Hook and Operators #5402

merged 1 commit into from Aug 7, 2019

Conversation

ryanyuan
Copy link
Contributor

@ryanyuan ryanyuan commented Jun 10, 2019 •
edited

[AIRFLOW-4746] Implement GCP Cloud Tasks' Hook and Operators

Make sure you have checked all steps below.

Jira

  • My PR addresses the following Airflow-4726 issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR"
    • https://issues.apache.org/jira/browse/AIRFLOW-XXX
    • In case you are fixing a typo in the documentation you can prepend your commit with [AIRFLOW-XXX], code changes always need a Jira issue.
    • In case you are proposing a fundamental code change, you need to create an Airflow Improvement Proposal (AIP).
    • In case you are adding a dependency, check if the license complies with the ASF 3rd Party License Policy.

Description

  • Here are some details about my PR, including screenshots of any UI changes:
    Implement hook and operators for GCP Cloud Tasks.

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:
    tests.contrib.operators.test_gcp_tasks_operator.py
    tests.contrib.operators.test_gcp_tasks_operator_system.py
    tests.contrib.hooks.test_gcp_tasks_hook.py

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

Code Quality

  • Passes flake8

@mik-laj
Copy link
Member

mik-laj commented Jun 11, 2019

Pylint is sad

Running pylint for source code without tests
************* Module airflow.contrib.hooks.gcp_tasks_hook
airflow/contrib/hooks/gcp_tasks_hook.py:1:0: C0111: Missing module docstring (missing-docstring)
airflow/contrib/hooks/gcp_tasks_hook.py:44:4: W0235: Useless super delegation in method '__init__' (useless-super-delegation)
************* Module airflow.contrib.operators.gcp_tasks_operator
airflow/contrib/operators/gcp_tasks_operator.py:1:0: C0111: Missing module docstring (missing-docstring)
airflow/contrib/operators/gcp_tasks_operator.py:600:4: R0913: Too many arguments (11/10) (too-many-arguments)
-----------------------------------
Your code has been rated at 9.97/10
Running pylint for tests
************* Module tests.contrib.operators.test_gcp_tasks_operator
tests/contrib/operators/test_gcp_tasks_operator.py:54:8: C0103: Variable name "op" doesn't conform to snake_case naming style (invalid-name)
tests/contrib/operators/test_gcp_tasks_operator.py:74:8: C0103: Variable name "op" doesn't conform to snake_case naming style (invalid-name)
tests/contrib/operators/test_gcp_tasks_operator.py:95:8: C0103: Variable name "op" doesn't conform to snake_case naming style (invalid-name)
tests/contrib/operators/test_gcp_tasks_operator.py:114:8: C0103: Variable name "op" doesn't conform to snake_case naming style (invalid-name)
tests/contrib/operators/test_gcp_tasks_operator.py:132:8: C0103: Variable name "op" doesn't conform to snake_case naming style (invalid-name)
tests/contrib/operators/test_gcp_tasks_operator.py:151:8: C0103: Variable name "op" doesn't conform to snake_case naming style (invalid-name)
tests/contrib/operators/test_gcp_tasks_operator.py:170:8: C0103: Variable name "op" doesn't conform to snake_case naming style (invalid-name)
tests/contrib/operators/test_gcp_tasks_operator.py:189:8: C0103: Variable name "op" doesn't conform to snake_case naming style (invalid-name)
tests/contrib/operators/test_gcp_tasks_operator.py:208:8: C0103: Variable name "op" doesn't conform to snake_case naming style (invalid-name)
tests/contrib/operators/test_gcp_tasks_operator.py:230:8: C0103: Variable name "op" doesn't conform to snake_case naming style (invalid-name)
tests/contrib/operators/test_gcp_tasks_operator.py:251:8: C0103: Variable name "op" doesn't conform to snake_case naming style (invalid-name)
tests/contrib/operators/test_gcp_tasks_operator.py:272:8: C0103: Variable name "op" doesn't conform to snake_case naming style (invalid-name)
tests/contrib/operators/test_gcp_tasks_operator.py:292:8: C0103: Variable name "op" doesn't conform to snake_case naming style (invalid-name)
-----------------------------------
Your code has been rated at 9.36/10

@ryanyuan
Copy link
Contributor Author

ryanyuan commented Jun 11, 2019 •
edited

@mik-laj @kaxil @potiuk PTAL

@codecov-io
Copy link

codecov-io commented Jun 12, 2019 •
edited

Codecov Report

❗ No coverage uploaded for pull request base (master@098b78d). Click here to learn what that means.
The diff coverage is 90.75%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #5402   +/-   ##
=========================================
  Coverage          ?   79.21%           
=========================================
  Files             ?      486           
  Lines             ?    30630           
  Branches          ?        0           
=========================================
  Hits              ?    24263           
  Misses            ?     6367           
  Partials          ?        0
Impacted Files Coverage Δ
airflow/contrib/example_dags/example_gcp_tasks.py 0% <0%> (ø)
airflow/contrib/operators/gcp_tasks_operator.py 100% <100%> (ø)
airflow/contrib/hooks/gcp_tasks_hook.py 89.38% <89.38%> (ø)

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 098b78d...f0de000. Read the comment docs.

@mik-laj
Copy link
Member

mik-laj commented Jun 12, 2019

@ryanyuan Is everything alright now with Travis? Can I help you with something else?

@ryanyuan
Copy link
Contributor Author

@mik-laj Travis is good now.

@ryanyuan
Copy link
Contributor Author

ryanyuan commented Jul 1, 2019

PTAL @kaxil @potiuk @mik-laj

@mik-laj mik-laj added the provider:google Google (including GCP) related issues label Jul 22, 2019
@mik-laj mik-laj self-requested a review July 27, 2019 14:53
@turbaszek turbaszek mentioned this pull request Aug 1, 2019
Closed
@potiuk
Copy link
Member

potiuk commented Aug 5, 2019

@ryanyuan -> can you please rebase /remove conflict. I will be happy to merge this one!

@potiuk
Copy link
Member

potiuk commented Aug 5, 2019

Pylint :( ?

@ryanyuan ryanyuan force-pushed the gcp-tasks branch 3 times, most recently from 478df15 to 44a7f60 Compare August 6, 2019 02:47
@ryanyuan
Copy link
Contributor Author

ryanyuan commented Aug 6, 2019

@potiuk All fixes. Thanks

potiuk
potiuk reviewed Aug 6, 2019
View reviewed changes
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.

It all looks really good!
I have only one remaining thing to ask - if that's not too much. We have just added Client Info to the GCP operators/hooks that missed it . It was supposed to be there in all GCP operators (as per GCP Integration Guide). See here: #5728. It's super-simple to add and maybe you can still add it + rebase to latest setup.py changes to make sure it works with latest GCP libraries.

@mik-laj
Copy link
Member

mik-laj commented Aug 6, 2019 •
edited

I wonder about the names of these operators. Are they consistent with the recommendations?
Current: CloudTasksCreateQueueOperator

@potiuk WDYT?
My proposition: CloudTasksQueueCreateOperator

@ryanyuan
Copy link
Contributor Author

ryanyuan commented Aug 6, 2019

@mik-laj You are correct.

Reference: GCP Service Airflow Integration Guide
Section: Defining Operator Class

@ryanyuan
[AIRFLOW-4746] Implement GCP Cloud Tasks' Hook and Operators
f0de000 
Implement GCP Cloud Tasks' Hook and Operators
@ryanyuan
Copy link
Contributor Author

ryanyuan commented Aug 7, 2019

@potiuk PR rebased and updated. PTAL. Thank you.

@potiuk potiuk merged commit 7cdf99a into apache:master Aug 7, 2019
@potiuk
Copy link
Member

potiuk commented Aug 7, 2019

Thanks @ryanyuan -> great addition to GCP operators list :)

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

@potiuk potiuk potiuk left review comments

@mik-laj mik-laj Awaiting requested review from mik-laj

Assignees
No one assigned
Labels
provider:google Google (including GCP) related issues
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ryanyuan @mik-laj @codecov-io @potiuk