Implements `AwsBaseOperator` and `AwsBaseSensor` by Taragolis · Pull Request #34784 · apache/airflow

Taragolis · 2023-10-05T19:16:31Z

Continuation #24246

This base operators and sensors might help to resolve inconsistency in multiple operators and sensors:

Some of current sensors/operators accept only aws_conn_id
Some of current sensors/operators accept region instead of region_name

In additional this implementation propagate additional parameters into the hook:

Operator/Sensor botocore_config -> Hook config
Operator/Sensor verify -> Hook verify

Note

AwsBaseSensor created by "old methodic"

Copy code from AwsBaseOperator
Paste in new module

I've tried to create Mixin class however it not works well breaks something in BaseOperator
If someone know how to implements it by DRY, without use additional Metaclasses and/or class decorators feel free to suggest.

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

Taragolis · 2023-10-05T19:18:37Z

airflow/providers/amazon/aws/operators/base_aws.py

It is also might be a good idea to make this classes internal only.

And wondering about module naming because I a bit confused:

Suggestion in old PR use base_aws: #24246 (comment)
Naming for Base Trigger: airflow.providers.amazon.aws.triggers.base.AwsBaseWaiterTrigger

cc @ferruzzi @vincbeck @o-nikolas

There are only two hard things in Computer Science: cache invalidation and naming things

Agree, naming is hard xD. I am fine with base_aws.py. It could be also:

base_operator.py

base.py

It is also might be a good idea to make this classes internal only.

+1

Taragolis · 2023-10-05T19:30:42Z

airflow/providers/amazon/aws/operators/base_aws.py

This changes also required to make changes in airflow.providers.amazon.aws.triggers.base.AwsBaseWaiterTrigger as follow-up.

That is also why I make accept botocore_config as dictionary and not botocore.config.Config object

ferruzzi

Left some suggestions. We've discussed an AwsBaseOperator a few times and kept putting it off, there may be some more suggestions coming once I check my notes on what I was going to put into it.

As always, thanks for your thought and effort on the provider package contribution. 👍

airflow/providers/amazon/aws/operators/base_aws.py

ferruzzi · 2023-10-05T19:36:37Z

airflow/providers/amazon/aws/operators/base_aws.py

I think this is repeating itself, but maybe I'm missing something.

Using the default AWS Connection id aws_default (as long as it is created and remains untouched by the user) also uses the default boto3 behaviour. Might be worth mentioning.

Maybe we could try to rewrite it to something shorter with reference to AWS Connection, like we have in other providers, e.g. Slack

airflow/airflow/providers/slack/operators/slack_webhook.py

Lines 42 to 43 in ed6a4fd

:param slack_webhook_conn_id: :ref:`Slack Incoming Webhook <howto/connection:slack>`

connection id that has Incoming Webhook token in the password field.

Example in Auto API:
https://airflow.apache.org/docs/apache-airflow-providers-slack/8.1.0/_api/airflow/providers/slack/operators/slack_webhook/index.html

airflow/providers/amazon/aws/operators/base_aws.py

airflow/providers/amazon/aws/sensors/base_aws.py

tests/providers/amazon/aws/operators/test_base_aws.py

airflow/providers/amazon/aws/hooks/base_aws.py

ferruzzi

Looks like my comments have all been addressed. Thanks.

eladkal

I assume followup PR would be to change AWS operators/hooks to inherit from the base classes?

We need also doc update to explain how to set the botocore parameters

eladkal · 2023-10-06T06:55:01Z

tests/always/test_project_structure.py

You need to move these to the BASE_CLASSES

I was a bit blind 🤣

Taragolis · 2023-10-06T13:15:14Z

I assume followup PR would be to change AWS operators/hooks to inherit from the base classes?

Yep

We need also doc update to explain how to set the botocore parameters

I think it would be a good idea to add generic information here: https://airflow.apache.org/docs/apache-airflow-providers-amazon/stable/operators/index.html as soon as we change existed Operators, so them will have same sets of generic parameters:

aws_conn_id: Operators which intends to use AwsBaseHook already have
region_name: Some of operators use this parameter
verify: Only one use right now
botocore_config: None of of the operators use it right now

vincbeck · 2023-10-06T13:22:08Z

airflow/providers/amazon/aws/operators/base_aws.py

Agree, naming is hard xD. I am fine with base_aws.py. It could be also:

base_operator.py

base.py

vincbeck · 2023-10-06T13:24:19Z

airflow/providers/amazon/aws/operators/base_aws.py

Out of curiosity. Why from None?

Just for skip previous traceback, e.g. if developer set aws_hook_class then it failed with TypeError on issubclass and traceback will contain information about error which raised from issubclass

I see, thanks for the explanation

I had to research that one too, I have never seen that in use. It's kinda handy.

airflow/providers/amazon/aws/operators/base_aws.py

Co-authored-by: D. Ferruzzi <[email protected]>

Taragolis · 2023-10-10T13:47:03Z

In 126a1f5 split logic into two classes, for reduce repeatable code in AwsBaseOperator and AwsBaseSensor and in additional this might help to use in other places, e.g. Notifiers. Potentially it also could be use in AwsBaseWaiterTrigger however it use hook as a method, rather than property

o-nikolas · 2023-10-10T19:33:05Z

airflow/providers/amazon/aws/operators/base_aws.py

+        Only for internal usage, this class might be changed, renamed or removed in the future
+        without any further notice.


Regarding this, should we change the class name to _AwsBaseOperator?

Same for the Sensor and mixin classes

There is kind of tradeoff between common Python's agreement about internal variables/classes/methods and airflow public models. _ shows that is internal stuff however IDE doesn't like this imports even with the same package (I guess static checks is fine) in the other hand we have some methods and class in core which do no prefixed by _ but have a description and :meta private:.

There is also maybe a good idea to make this stuff public after a while for create custom operators, but for now I would rather keep it internal only

In the end no one could stop end users to use this stuff directly

o-nikolas · 2023-10-10T19:36:32Z

airflow/providers/amazon/aws/operators/base_aws.py

Using the default AWS Connection id aws_default (as long as it is created and remains untouched by the user) also uses the default boto3 behaviour. Might be worth mentioning.

airflow/providers/amazon/aws/utils/mixins.py

Co-authored-by: Niko Oliveira <[email protected]>

syedahsn

Looks good to me. I'd be interested to see how much this affects new and existing operators and sensors.

eladkal

LGTM!

Taragolis requested review from ferruzzi and vincbeck October 5, 2023 19:16

Taragolis requested review from eladkal and o-nikolas as code owners October 5, 2023 19:16

boring-cyborg bot added area:providers provider:amazon AWS/Amazon - related issues labels Oct 5, 2023

Taragolis commented Oct 5, 2023

View reviewed changes

ferruzzi reviewed Oct 5, 2023

View reviewed changes

Taragolis commented Oct 5, 2023

View reviewed changes

airflow/providers/amazon/aws/hooks/base_aws.py Outdated Show resolved Hide resolved

ferruzzi approved these changes Oct 5, 2023

View reviewed changes

eladkal reviewed Oct 6, 2023

View reviewed changes

TimBogevich mentioned this pull request Oct 6, 2023

12th commit igormurigor/eapteka-interview-tests#1

Merged

Taragolis mentioned this pull request Oct 6, 2023

Do not mock isinstance in Amazon Tests #34800

Merged

vincbeck reviewed Oct 6, 2023

View reviewed changes

Taragolis force-pushed the amazon-base-operator-sensors branch from d38cde3 to 63052fa Compare October 10, 2023 09:14

Taragolis and others added 7 commits October 10, 2023 17:07

Implements AwsBaseOperator and AwsBaseSensor

6ba02a2

Apply suggestions from code review

5b9fde0

Co-authored-by: D. Ferruzzi <[email protected]>

Move suggestion into the AwsBaseSensor

89817cf

Static checks + tests

cda6a34

Move to base classes

e70631f

Change to base python exceptions

955ceea

Add mixin helpers

126a1f5

Taragolis force-pushed the amazon-base-operator-sensors branch from 63052fa to 126a1f5 Compare October 10, 2023 13:09

vincbeck approved these changes Oct 10, 2023

View reviewed changes

o-nikolas reviewed Oct 10, 2023

View reviewed changes

Update airflow/providers/amazon/aws/utils/mixins.py

e480479

Co-authored-by: Niko Oliveira <[email protected]>

syedahsn approved these changes Oct 11, 2023

View reviewed changes

eladkal approved these changes Oct 11, 2023

View reviewed changes

o-nikolas approved these changes Oct 11, 2023

View reviewed changes

ferruzzi merged commit 84a3dae into apache:main Oct 11, 2023

Taragolis deleted the amazon-base-operator-sensors branch October 12, 2023 00:18

Taragolis mentioned this pull request Oct 12, 2023

Extend hooks arguments into AwsBaseWaiterTrigger #34884

Merged

eladkal mentioned this pull request Oct 13, 2023

Status of testing Providers that were prepared on October 13, 2023 #34930

Closed

98 tasks

Taragolis mentioned this pull request Oct 30, 2023

Use base aws classes in amazon provider Operators/Sensors/Triggers #35278

Open

28 tasks

	:param slack_webhook_conn_id: :ref:`Slack Incoming Webhook <howto/connection:slack>`
	connection id that has Incoming Webhook token in the password field.

		Only for internal usage, this class might be changed, renamed or removed in the future
		without any further notice.

Conversation

Taragolis commented Oct 5, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ferruzzi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ferruzzi Oct 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ferruzzi left a comment

Choose a reason for hiding this comment

Uh oh!

eladkal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Taragolis commented Oct 6, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Taragolis commented Oct 10, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

syedahsn left a comment

Choose a reason for hiding this comment

Uh oh!

eladkal left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

ferruzzi Oct 5, 2023 •

edited

Loading