Remove EMR Conn from example #525
Conversation
pankajastro
requested review from
phanikumv,
bharanidharan14,
rajaths010494 and
pankajkoti
as code owners
Codecov Report
@@ Coverage Diff @@
## main #525 +/- ##
=======================================
Coverage 98.27% 98.27%
=======================================
Files 72 72
Lines 3996 3996
=======================================
Hits 3927 3927
Misses 69 69 Continue to review full report at Codecov.
|
| @@ -72,7 +71,7 @@ | |||
| cluster_creator = EmrCreateJobFlowOperator( | |||
| task_id="create_job_flow", | |||
| job_flow_overrides=JOB_FLOW_OVERRIDES, | |||
| emr_conn_id=EMR_CONN_ID, | |||
| aws_conn_id=AWS_CONN_ID, | |||
There was a problem hiding this comment.
will this fail for apache-airflow-providers-amazon < 4.1.0 ? We should make it fail safe for previous versions of the provider then. Something similar to this, with a try-except block catching the exception: #471 (comment)
There was a problem hiding this comment.
hmm, it would fail for apache-airflow-providers-amazon < 4.1.0. Does it make sense to safe fail here since this is an integration test?
There was a problem hiding this comment.
I would recommend to add try-except block to fail safe for previous versions as well as @pankajkoti suggested even though it is in integration
There was a problem hiding this comment.
@phanikumv @bharanidharan14 what is your opinion on this?
There was a problem hiding this comment.
@pankajkoti I would recommend to add try-except block. I have mentioned it already
There was a problem hiding this comment.
I am not sure if we can use try-catch for an argument being passed to the operator.
There was a problem hiding this comment.
Perhaps we can add a comment in the example DAG that based on the versions of the provider what arguments are expected
There was a problem hiding this comment.
@pankajastro adding comments in the example DAG is reasonable to me, so it helps user to know what arguements needs to be passed
There was a problem hiding this comment.
@pankajastro Is this review comment is addressed ?
There was a problem hiding this comment.
yes, I have added a comment.
pankajastro
force-pushed
the
remove_emr_conn_id_from_example
branch
from
0ae6bf3 to
aeabf65
Compare
Now
emr_conn_idis optional in EmrCreateJobFlowOperator since the configuration we pass using paramjob_flow_overridesso theemr_conn_idis no longer require. We can merge this once the provider for June is releasedapache-airflow-providers-amazon==4.1.0OSS PR: apache/airflow#24306