Standardize naming of AWS Sqs #20418

KennyRich · 2021-12-20T07:16:44Z

Part of #20296

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.

eladkal

Did partial review as some of my comments apply to more than 1 file.

Let me know if something isn't clear enough

eladkal · 2021-12-20T07:23:16Z

CHANGELOG.txt

 - [AIRFLOW-XXX] Ignore Python files under node_modules in docs (#5063)
 - [AIRFLOW-XXX] Build a universal wheel with LICNESE files (#5052)
- [AIRFLOW-XXX] Fix docstrings of SQSHook (#5099)
+- [AIRFLOW-XXX] Fix docstrings of SqsHook (#5099)


We don't alter past records in change log. this entry was valid to the day it was created.
Please revert this change.

eladkal · 2021-12-20T07:24:34Z

UPDATING.md

 | airflow.contrib.hooks.aws_athena_hook.AWSAthenaHook          | airflow.providers.amazon.aws.hooks.athena.AWSAthenaHook         |
 | airflow.contrib.hooks.aws_lambda_hook.AwsLambdaHook          | airflow.providers.amazon.aws.hooks.lambda_function.AwsLambdaHook         |
-| airflow.contrib.hooks.aws_sqs_hook.SQSHook                   | airflow.providers.amazon.aws.hooks.sqs.SQSHook        |
+| airflow.contrib.hooks.aws_sqs_hook.SqsHook                   | airflow.providers.amazon.aws.hooks.sqs.SqsHook        |


There is no SqsHook in contrib.
We also can't suggest here airflow.contrib.hooks.aws_sqs_hook.SQSHook -> airflow.providers.amazon.aws.hooks.sqs.SqsHook because this table is for upgrading from 1.10 to 2.0 and in 2.0 only backport amazon provider is compatible so the changes we do in this PR are not relevant for it.

Long story short - please undo the changes you made for this file :)

I still see changes on this file
@KennyRich did you undo all changes to this file?

eladkal · 2021-12-20T07:30:26Z

airflow/contrib/hooks/aws_sqs_hook.py

 import warnings

-from airflow.providers.amazon.aws.hooks.sqs import SQSHook  # noqa
+from airflow.providers.amazon.aws.hooks.sqs import SqsHook  # noqa


This is not backward compatible. We are changing the class name.
So with the current code when user try to import SQSHook it won't work
Check reference of SESHook https://github.com/apache/airflow/pull/20367/files

It should

Thanks. I understand that some of the changes aren't backward compatible but, this file has a deprecation warning here already. Do I just revert the changes here and create a deprecation warning for the SQSHook and make it backward compatible like this

class SQSHook(SqsHook): """ This hook is deprecated. Please use :class:`airflow.providers.amazon.aws.hooks.sqs.SqsHook`. """ def __init__(self, *args, **kwargs): warnings.warn( "This hook is deprecated. " "Please use :class:`airflow.providers.amazon.aws.hooks.sqs.SqsHook`.", DeprecationWarning, stacklevel=2, ) super().__init__(*args, **kwargs)

yes. The current implementation support deprecation where the class name was not change so it just add an import of where to find it but now - the class name did change so we must address it. Yes the class you wrote should do the trick.

eladkal · 2021-12-20T07:30:43Z

airflow/contrib/operators/aws_sqs_publish_operator.py

 import warnings

-from airflow.providers.amazon.aws.operators.sqs import SQSPublishOperator  # noqa
+from airflow.providers.amazon.aws.operators.sqs import SqsPublishOperator  # noqa


eladkal · 2021-12-20T07:30:52Z

airflow/contrib/sensors/aws_sqs_sensor.py

 import warnings

-from airflow.providers.amazon.aws.sensors.sqs import SQSSensor  # noqa
+from airflow.providers.amazon.aws.sensors.sqs import SqsSensor  # noqa


eladkal · 2021-12-20T07:31:16Z

airflow/providers/amazon/CHANGELOG.rst

 * ``Fix AthenaSensor calling AthenaHook incorrectly (#15427)``
 * ``Add links to new modules for deprecated modules (#15316)``
-* ``Fixes doc for SQSSensor (#15323)``
+* ``Fixes doc for SqsSensor (#15323)``


same comment as previous changelog.
Please undo

eladkal · 2021-12-20T07:32:22Z

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



-class SQSHook(AwsBaseHook):
+class SqsHook(AwsBaseHook):


This is also not backward compatible. We want both SQSHook and SqsHook to work.

This has been handled please check and let me know what you think.

eladkal · 2021-12-20T09:13:20Z

docs/apache-airflow-providers-amazon/operators/sqs_publish.rst



-.. _howto/operator:SQSPublishOperator:
+.. _howto/operator:operator:SQSPublishOperator:


Is this right ? Shouldn't it be?

Suggested change

.. _howto/operator:operator:SQSPublishOperator:

.. _howto/operator:SqsPublishOperator:

eladkal · 2021-12-20T09:13:27Z

docs/apache-airflow-providers-amazon/operators/sqs_publish.rst


 Use the
-:class:`~airflow.providers.amazon.aws.operators.sqs.SQSPublishOperator`
+:class:`~airflow.providers.amazon.aws.operators.sqs.operator:SQSPublishOperator`


same question

eladkal · 2021-12-20T09:14:36Z

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


 from airflow.providers.amazon.aws.hooks.s3 import S3Hook
-from airflow.providers.amazon.aws.operators.s3 import S3CreateBucketOperator, S3DeleteBucketOperator
+from airflow.providers.amazon.aws.operators.s3_bucket import S3CreateBucketOperator, S3DeleteBucketOperator


Why are you modifying s3 files?
I think you have several unrelated changes of s3 in this PR.

eladkal · 2022-01-04T12:24:36Z

@KennyRich do you need help to finish this PR?

KennyRich · 2022-01-04T12:37:13Z

@KennyRich do you need help to finish this PR?

Yes please, I have been unwell for a couple of days now

eladkal · 2022-01-06T22:17:30Z

Closing in favor of #20732

KennyRich added 5 commits December 20, 2021 07:46

Task: Organize S3 Classes in Amazon Provider

ab39652

Task: Organize S3 Classes in Amazon Provider

db589c1

Task: Organize S3 Classes in Amazon Provider

391220c

Merge upstream into local

1a6709f

Task: Standardize naming of AWS Sqs

0733dd0

KennyRich requested review from kaxil, mik-laj, potiuk and vikramkoka as code owners December 20, 2021 07:16

boring-cyborg bot added area:providers kind:documentation provider:amazon AWS/Amazon - related issues labels Dec 20, 2021

eladkal requested changes Dec 20, 2021

View reviewed changes

eladkal mentioned this pull request Dec 20, 2021

Consistently name Amazon provider operators files #20296

Closed

23 tasks

KennyRich added 3 commits December 20, 2021 09:09

Task: Standardize naming of AWS Sqs

385d733

Task: Standardize naming of AWS Sqs

5f90498

Task: Fix static check errors

fca10db

eladkal requested changes Dec 20, 2021

View reviewed changes

eladkal removed request for kaxil, mik-laj, potiuk and vikramkoka December 20, 2021 20:07

eladkal mentioned this pull request Jan 6, 2022

Standardize AWS SQS classes names #20732

Merged

eladkal closed this Jan 6, 2022



		.. _howto/operator:SQSPublishOperator:
		.. _howto/operator:operator:SQSPublishOperator:

Standardize naming of AWS Sqs #20418

Standardize naming of AWS Sqs #20418

Uh oh!

Conversation

KennyRich commented Dec 20, 2021

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!

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!

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!

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!

eladkal commented Jan 4, 2022

Uh oh!

KennyRich commented Jan 4, 2022

Uh oh!

eladkal commented Jan 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants