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.

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

[AIRFLOW-1695] Add RedshiftHook using boto3 #2717

Closed
wants to merge 1 commit into from

Conversation

andyxhadji
Copy link
Contributor

@andyxhadji andyxhadji commented Oct 22, 2017

Dear Airflow maintainers,

Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!

JIRA

Description

  • Here are some details about my PR, including screenshots of any UI changes:
  • Added RedshiftHook using boto3, and tests.

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:
  • Added test_redshift_hook.py - includes unit tests for methods in class

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
    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"

@codecov-io
Copy link

codecov-io commented Oct 23, 2017

Codecov Report

Merging #2717 into master will increase coverage by 0.08%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2717      +/-   ##
==========================================
+ Coverage   72.48%   72.56%   +0.08%     
==========================================
  Files         154      154              
  Lines       11843    11843              
==========================================
+ Hits         8584     8594      +10     
+ Misses       3259     3249      -10
Impacted Files Coverage Δ
airflow/hooks/__init__.py 58.82% <ø> (ø) ⬆️
airflow/models.py 87.09% <0%> (-0.05%) ⬇️
airflow/jobs.py 79.66% <0%> (+0.44%) ⬆️
airflow/utils/helpers.py 56.32% <0%> (+2.87%) ⬆️
airflow/task_runner/bash_task_runner.py 100% <0%> (+6.66%) ⬆️

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 b3c247d...85d7e2d. Read the comment docs.

@andyxhadji andyxhadji changed the title [AIRFLOW-1695] RedshiftHook using boto3 [AIRFLOW-1695] Build RedshiftHook using boto3 Oct 23, 2017
@bolkedebruin
Copy link
Contributor

Is this reviewable? If so, please fix your commit message to make it readable. See commit guidelines. Furthermore please make sure the tests actually can run, include the right packages for ci.

@andyxhadji andyxhadji changed the title [AIRFLOW-1695] Build RedshiftHook using boto3 [AIRFLOW-1695] Add RedshiftHook using boto3 Oct 23, 2017
@sid88in
Copy link
Contributor

sid88in commented Oct 23, 2017

thanks @andyxhadji more power to #airflow and #aws

@andyxhadji
Copy link
Contributor Author

@bolkedebruin This is now reviewable, all tests are passing. Please take a look!

@andyxhadji
Copy link
Contributor Author

This is one of the follow-ups to #2532

Copy link
Contributor

@bolkedebruin bolkedebruin left a comment

Choose a reason for hiding this comment

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

Small nits. I don't think there is a need to touch aws_hook.py

self.aws_conn_id = aws_conn_id
self.region_name = region_name
Copy link
Contributor

Choose a reason for hiding this comment

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

This is obtained from the extra connectin parameters, why have it in the signature?

@bolkedebruin
Copy link
Contributor

Please also make sure your commit message adheres to the layout (descr lines are too long)

Adds RedshiftHook class, allowing for management of AWS Redshift
clusters and snapshots using boto3 library. Also adds new test file and
unit tests for class methods.
@andyxhadji
Copy link
Contributor Author

@bolkedebruin Fixed - re-running tests because of an unrelated failure (https://travis-ci.org/apache/incubator-airflow/builds/294151528).

@andyxhadji
Copy link
Contributor Author

@bolkedebruin Tests passing!

@bolkedebruin
Copy link
Contributor

Your tests are not running as they are not prepended by "test_". Please verify in the travis that your tests run.

@andyxhadji
Copy link
Contributor Author

@bolkedebruin Hmm, I'm not sure if I'm looking in the right place - the tests are prepended with "test_" and the tests run (screenshot from changed files & apache TravisCI below)
image

image

Am I missing something in a file? I really appreciate the help.

@bolkedebruin
Copy link
Contributor

Apologies! I did do a search but I probably misspelled.

asfgit pushed a commit that referenced this pull request Oct 30, 2017
Adds RedshiftHook class, allowing for management
of AWS Redshift
clusters and snapshots using boto3 library. Also
adds new test file and
unit tests for class methods.

Closes #2717 from andyxhadji/1695

(cherry picked from commit 4fb7a90)
Signed-off-by: Bolke de Bruin <[email protected]>
@asfgit asfgit closed this in 4fb7a90 Oct 30, 2017
@andyxhadji
Copy link
Contributor Author

andyxhadji commented Oct 30, 2017 via email

@andyxhadji andyxhadji deleted the 1695 branch October 30, 2017 19:39
Acehaidrey pushed a commit to Acehaidrey/incubator-airflow that referenced this pull request Jan 19, 2018
Adds RedshiftHook class, allowing for management
of AWS Redshift
clusters and snapshots using boto3 library. Also
adds new test file and
unit tests for class methods.

Closes apache#2717 from andyxhadji/1695
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.

4 participants