[AIRFLOW-3698] Add documentation for AWS Connection #4514

danabananarama · 2019-01-13T21:59:14Z

Make sure you have checked all steps below.

Jira

My PR addresses the following Airflow Jira issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR"
- https://issues.apache.org/jira/browse/AIRFLOW-3698

Description

Here are some details about my PR, including screenshots of any UI changes:
~~This PR is to allow setting the AWS region in the EmrJobFlowSensor so that we can sense EMR job flow completion in any region.~~
The AWS region setting functionality is available via the extra param in the Connection class, so adding a section in the docs to describe this

Tests

My PR adds the following unit tests OR does not need testing for this extremely good reason:
~~Amending test, did not add new test for functionality as it's essentially just persisting a new init param. Let me know if a new test needs to be added!~~
Doc change only

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

ashb

I'm not sure this is required - it is possibly to specify the region via the extra field of the connection: {"region_name": "ap-southeast-2"}

danabananarama · 2019-01-14T23:08:48Z

Thanks for looking @ashb! And thanks for the tip, it does the trick. Did I miss this in the documentation? (or if not, where would you recommend as a sensible place to add it? Here?)

ashb · 2019-01-15T20:52:40Z

You probably didn't miss it - it's a hole in the docs :(

danabananarama · 2019-01-16T23:53:53Z

I've updated the PR to instead add a bit in the Managing Connections page in the docs, please let me know if there's a more suitable home for it or if anything needs rewording (:

mik-laj · 2019-01-17T14:56:17Z

Could you provide a description on how to set up a connection via environment variables?
Similar to GCP: https://github.com/apache/airflow/pull/4523/files

danabananarama · 2019-01-17T22:18:33Z

@mik-laj Oh cool, I didn't know you could do that! I'm afraid I haven't set up the AWS Connection that way before so I'm not aware of any AWS-specific things you might need to specify when setting it up that way.

mik-laj · 2019-01-18T00:08:47Z

@mik-laj What specific things do you need?
schema part must be equals aws. Query parameters must cotanins extra parameters. For example: aws_access_key_id=AA&aws_secret_access_key=BB&CC=aws_account_id'. You should also write what the default con_id` is in hooks and operators for this platforms.

danabananarama · 2019-01-18T00:37:45Z

Since the query parameter bit is the same as for other Connection types, perhaps it makes more sense for the general case to be documented in the "Creating a Connection with Environment Variables" section to avoid repetition?

I'll add the general default conn_id, but I'm not sure it's worth including all of the hooks and operators for this, there are quite a lot and it would be tough to ensure this doc section is completely maintained going forward.

mik-laj · 2019-01-18T00:44:55Z

This is already described when my PR is accepted.I personally think that this is not enough. Unfortunately people are lazy and if they are to write a long command, they will rather look for it, only to copy it. I see it myself. It's easy to make a mistake, so I type in Google to get a full and ready command.

mik-laj · 2019-01-18T00:53:51Z

I do not think that it is worth writing a list of operators and hooks. just mention the default conn_id is enough

docs/howto/manage-connections.rst

XD-DENG · 2019-01-21T16:29:48Z

docs/howto/manage-connections.rst

+Password (optional)
+    Specify the AWS secret access key.
+
+Extra (optional)


it may be good to mention that users can specify endpoint in Extra section by giving "host". This will be useful in cases like they're using S3-compatible services (but not AWS service.)

ah I missed that, thanks for the spot!

danabananarama · 2019-01-21T21:55:36Z

Thanks for the feedback @ashb, @XD-DENG , I think I've addressed all the comments. Please let me know if anything else needs tweaking!

codecov-io · 2019-01-21T22:39:26Z

Codecov Report

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

@@            Coverage Diff             @@
##           master    #4514      +/-   ##
==========================================
+ Coverage   73.81%   74.12%   +0.31%     
==========================================
  Files         421      421              
  Lines       27662    27700      +38     
==========================================
+ Hits        20418    20532     +114     
+ Misses       7244     7168      -76

Impacted Files	Coverage Δ
airflow/bin/cli.py	`64.64% <0%> (-0.44%)`	⬇️
airflow/contrib/hooks/gcp_dataflow_hook.py	`75.75% <0%> (ø)`	⬆️
airflow/contrib/hooks/spark_submit_hook.py	`79.82% <0%> (+0.08%)`	⬆️
airflow/models/__init__.py	`92.33% <0%> (+0.14%)`	⬆️
airflow/contrib/operators/spark_submit_operator.py	`92.3% <0%> (+0.2%)`	⬆️
airflow/hooks/dbapi_hook.py	`79.03% <0%> (+0.8%)`	⬆️
airflow/hooks/hive_hooks.py	`75.26% <0%> (+1.84%)`	⬆️
airflow/models/connection.py	`64.02% <0%> (+1.99%)`	⬆️
airflow/contrib/hooks/gcp_api_base_hook.py	`81.39% <0%> (+3.78%)`	⬆️
airflow/utils/sqlalchemy.py	`81.81% <0%> (+4.54%)`	⬆️
... and 2 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 c0949cb...742914d. Read the comment docs.

codecov-io · 2019-01-21T22:39:27Z

Codecov Report

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

@@            Coverage Diff             @@
##           master    #4514      +/-   ##
==========================================
+ Coverage   73.81%   74.12%   +0.31%     
==========================================
  Files         421      421              
  Lines       27662    27700      +38     
==========================================
+ Hits        20418    20532     +114     
+ Misses       7244     7168      -76

Impacted Files	Coverage Δ
airflow/bin/cli.py	`64.64% <0%> (-0.44%)`	⬇️
airflow/contrib/hooks/gcp_dataflow_hook.py	`75.75% <0%> (ø)`	⬆️
airflow/contrib/hooks/spark_submit_hook.py	`79.82% <0%> (+0.08%)`	⬆️
airflow/models/__init__.py	`92.33% <0%> (+0.14%)`	⬆️
airflow/contrib/operators/spark_submit_operator.py	`92.3% <0%> (+0.2%)`	⬆️
airflow/hooks/dbapi_hook.py	`79.03% <0%> (+0.8%)`	⬆️
airflow/hooks/hive_hooks.py	`75.26% <0%> (+1.84%)`	⬆️
airflow/models/connection.py	`64.02% <0%> (+1.99%)`	⬆️
airflow/contrib/hooks/gcp_api_base_hook.py	`81.39% <0%> (+3.78%)`	⬆️
airflow/utils/sqlalchemy.py	`81.81% <0%> (+4.54%)`	⬆️
... and 2 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 c0949cb...742914d. Read the comment docs.

ashb

I'm happy. Any further comments @XD-DENG?

XD-DENG · 2019-01-22T16:31:01Z

@ashb , LGTM.

Thanks @danabananarama

danabananarama force-pushed the AIRFLOW-3698_set_jobflow_sensor_region branch 3 times, most recently from bb98cc2 to 46b95a8 Compare January 14, 2019 02:40

ashb reviewed Jan 14, 2019

View reviewed changes

[AIRFLOW-3698] Add documentation for AWS Connection type

e9e204a

danabananarama force-pushed the AIRFLOW-3698_set_jobflow_sensor_region branch from 46b95a8 to e9e204a Compare January 16, 2019 23:52

danabananarama changed the title ~~[AIRFLOW-3698] Add region param for EMR jobflow sensor~~ [AIRFLOW-3698] Add documentation for AWS Connection Jan 16, 2019

[AIRFLOW-3698] Add AWS connection default ID to docs

4d00963

ashb requested changes Jan 21, 2019

View reviewed changes

docs/howto/manage-connections.rst Outdated Show resolved Hide resolved

docs/howto/manage-connections.rst Show resolved Hide resolved

docs/howto/manage-connections.rst Outdated Show resolved Hide resolved

XD-DENG reviewed Jan 21, 2019

View reviewed changes

[AIRFLOW-3698] Update documentation for AWS connection parameters

742914d

ashb approved these changes Jan 22, 2019

View reviewed changes

XD-DENG approved these changes Jan 22, 2019

View reviewed changes

ashb merged commit 89dd877 into apache:master Jan 22, 2019

ashb pushed a commit that referenced this pull request Feb 20, 2019

[AIRFLOW-3698] Add documentation for AWS Connection (#4514)

949ac40

ashb pushed a commit that referenced this pull request Feb 21, 2019

[AIRFLOW-3698] Add documentation for AWS Connection (#4514)

397e06f

wmorris75 pushed a commit to modmed/incubator-airflow that referenced this pull request Jul 29, 2019

[AIRFLOW-3698] Add documentation for AWS Connection (apache#4514)

4dbf5c6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AIRFLOW-3698] Add documentation for AWS Connection #4514

[AIRFLOW-3698] Add documentation for AWS Connection #4514

danabananarama commented Jan 13, 2019 •

edited

Loading

ashb left a comment

danabananarama commented Jan 14, 2019

ashb commented Jan 15, 2019 •

edited

Loading

danabananarama commented Jan 16, 2019 •

edited

Loading

mik-laj commented Jan 17, 2019

danabananarama commented Jan 17, 2019

mik-laj commented Jan 18, 2019

danabananarama commented Jan 18, 2019

mik-laj commented Jan 18, 2019 •

edited

Loading

mik-laj commented Jan 18, 2019

XD-DENG Jan 21, 2019

danabananarama Jan 21, 2019

danabananarama commented Jan 21, 2019

codecov-io commented Jan 21, 2019

codecov-io commented Jan 21, 2019

ashb left a comment

XD-DENG commented Jan 22, 2019

[AIRFLOW-3698] Add documentation for AWS Connection #4514

[AIRFLOW-3698] Add documentation for AWS Connection #4514

Conversation

danabananarama commented Jan 13, 2019 • edited Loading

Jira

Description

Tests

Commits

Documentation

Code Quality

ashb left a comment

Choose a reason for hiding this comment

danabananarama commented Jan 14, 2019

ashb commented Jan 15, 2019 • edited Loading

danabananarama commented Jan 16, 2019 • edited Loading

mik-laj commented Jan 17, 2019

danabananarama commented Jan 17, 2019

mik-laj commented Jan 18, 2019

danabananarama commented Jan 18, 2019

mik-laj commented Jan 18, 2019 • edited Loading

mik-laj commented Jan 18, 2019

XD-DENG Jan 21, 2019

Choose a reason for hiding this comment

danabananarama Jan 21, 2019

Choose a reason for hiding this comment

danabananarama commented Jan 21, 2019

codecov-io commented Jan 21, 2019

Codecov Report

codecov-io commented Jan 21, 2019

Codecov Report

ashb left a comment

Choose a reason for hiding this comment

XD-DENG commented Jan 22, 2019

danabananarama commented Jan 13, 2019 •

edited

Loading

ashb commented Jan 15, 2019 •

edited

Loading

danabananarama commented Jan 16, 2019 •

edited

Loading

mik-laj commented Jan 18, 2019 •

edited

Loading