vincbeck commented on code in PR #33408:
URL: https://github.com/apache/airflow/pull/33408#discussion_r1294716332
##########
airflow/providers/amazon/aws/operators/glue.py:
##########
@@ -53,7 +53,8 @@ class GlueJobOperator(BaseOperator):
:param num_of_dpus: Number of AWS Glue DPUs to allocate to this Job.
:param region_name: aws region name (example: us-east-1)
:param s3_bucket: S3 bucket where logs and local etl script will be
uploaded
- :param iam_role_name: AWS IAM Role for Glue Job Execution
+ :param iam_role_name: AWS IAM Role for Glue Job Execution. If set
iam_role_arn must equal None.
+ :param iam_role_arn: AWS IAM ARN for Glue Job Execution. If set
iam_role_name must equal None.
Review Comment:
```suggestion
:param iam_role_name: AWS IAM Role for Glue Job Execution. If set
`iam_role_arn`, must equal None.
:param iam_role_arn: AWS IAM ARN for Glue Job Execution. If set
`iam_role_name`, must equal None.
```
##########
airflow/providers/amazon/aws/hooks/glue.py:
##########
@@ -44,7 +44,8 @@ class GlueJobHook(AwsBaseHook):
:param retry_limit: Maximum number of times to retry this job if it fails
:param num_of_dpus: Number of AWS Glue DPUs to allocate to this Job
:param region_name: aws region name (example: us-east-1)
- :param iam_role_name: AWS IAM Role for Glue Job Execution
+ :param iam_role_name: AWS IAM Role for Glue Job Execution. If set
iam_role_arn must equal None.
Review Comment:
```suggestion
:param iam_role_name: AWS IAM Role for Glue Job Execution. If set
`iam_role_arn`, must equal None.
```
##########
airflow/providers/amazon/aws/hooks/glue.py:
##########
@@ -114,12 +119,14 @@ def create_glue_job_config(self) -> dict:
"ScriptLocation": self.script_location,
}
command = self.create_job_kwargs.pop("Command", default_command)
- execution_role = self.get_iam_execution_role()
+ if not self.role_arn:
+ execution_role = self.get_iam_execution_role()
+ self.role_arn = execution_role["Role"]["Arn"]
Review Comment:
nit. There is no need to save it as instance variable, local variable is
enough
```suggestion
role_arn = execution_role["Role"]["Arn"]
```
##########
airflow/providers/amazon/aws/hooks/glue.py:
##########
@@ -44,7 +44,8 @@ class GlueJobHook(AwsBaseHook):
:param retry_limit: Maximum number of times to retry this job if it fails
:param num_of_dpus: Number of AWS Glue DPUs to allocate to this Job
:param region_name: aws region name (example: us-east-1)
- :param iam_role_name: AWS IAM Role for Glue Job Execution
+ :param iam_role_name: AWS IAM Role for Glue Job Execution. If set
iam_role_arn must equal None.
+ :param iam_role_arn: AWS IAM Role ARN for Glue Job Execution, If set
iam_role_name must equal None.
Review Comment:
```suggestion
:param iam_role_arn: AWS IAM Role ARN for Glue Job Execution, If set
`iam_role_name`, must equal None.
```
##########
tests/providers/amazon/aws/hooks/test_glue.py:
##########
@@ -90,6 +90,56 @@ class JobNotFoundException(Exception):
assert result is False
mock_conn.get_job.assert_called_once_with(JobName=job_name)
+ @mock.patch.object(GlueJobHook, "get_iam_execution_role")
+ @mock.patch.object(AwsBaseHook, "conn")
+ def test_role_arn_has_job_exists(self, mock_conn,
mock_get_iam_execution_role):
Review Comment:
Maybe add a test to test that if both parameters are provided, you get a
`ValueError`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
