[AIRFLOW-6206] Move and rename AWS batch operator [AIP-21]

[dazza-codes]

Jira

• My PR addresses the following [Airflow Jira]
  • https://issues.apache.org/jira/browse/AIRFLOW-6206
  • https://issues.apache.org/jira/browse/AIRFLOW-4733 [AIP-21]

Description

• basic description

Modules and classes for AWS batch are adapted to https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-21%3A+Changes+in+import+paths

Tests

• My PR does not change existing test success and it adds deprecation tests

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":

Documentation

• In case of new functionality, my PR adds documentation that describes how to use it.
  • existing documentation is patched to use the revised module name

[codecov-io]

Codecov Report

Merging #6764 into master will decrease coverage by 0.13%.
The diff coverage is 96.09%.

@@            Coverage Diff             @@
##           master    #6764      +/-   ##
==========================================
- Coverage   84.62%   84.48%   -0.14%     
==========================================
  Files         673      677       +4     
  Lines       38279    39076     +797     
==========================================
+ Hits        32392    33015     +623     
- Misses       5887     6061     +174

Impacted Files	Coverage Δ
airflow/contrib/operators/awsbatch_operator.py	100% <100%> (+4.16%)	⬆️
airflow/providers/amazon/aws/operators/batch.py	95.83% <95.83%> (ø)
airflow/kubernetes/volume_mount.py	44.44% <0%> (-55.56%)	⬇️
airflow/kubernetes/volume.py	52.94% <0%> (-47.06%)	⬇️
airflow/kubernetes/pod_launcher.py	45.25% <0%> (-46.72%)	⬇️
airflow/kubernetes/refresh_config.py	50.98% <0%> (-23.53%)	⬇️
...rflow/contrib/operators/kubernetes_pod_operator.py	78.2% <0%> (-20.52%)	⬇️
airflow/contrib/operators/jira_operator.py	71.87% <0%> (-4.8%)	⬇️
airflow/contrib/hooks/jira_hook.py	75.67% <0%> (-4.33%)	⬇️
airflow/contrib/sensors/jira_sensor.py	56.89% <0%> (-3.11%)	⬇️
... and 8 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 69ef1c6...c9dac4c. Read the comment docs.

[mik-laj]

Hi. Can you add backward compatibility and move the module to new package airflow.providers according to AIP-21? You should also add test cases in https://github.com/apache/airflow/blob/master/tests/test_core_to_contrib.py

[dazza-codes]

@mik-laj - With respect, adding those requirements onto this PR seems out of place. This PR is really only about renaming the module, the additional requirements for AIP-21 seem to belong elsewhere with a larger migration of many contribution modules? It seems out of place to do that migration just for this one module in this PR. AFAICT, the use of this module might not be changed because the import patterns might not refer directly to the full package path for this module, but get it via higher level packaging (or wrapping) and that is not changed here in this PR. If it is a breaking change, then it should be so because the naming pattern should be consistent across all the AWS providers (contrib modules).

Since the batch operator is not already in the https://github.com/apache/airflow/blob/master/tests/test_core_to_contrib.py, it was not found with a git-grep. It might be an oversight that it's not there already, or an intentional omission. Is there a clear answer as to why it is not already there? If it's a simple omission and it's easy to add a two-line mapping, I guess it could be added here in this PR. FYI, compare the outputs from

git grep 'AWSBatchOperator'
git grep 'AWSAthenaOperator'

You will see that the AWSBatchOperator is not already in the airflow/providers/amazon/aws/operators/ package and so adding it to that test fails the test. Based on that, my best guess is that it's an intentional omission from that test.

[mik-laj]

This is not just a module change, but your implementation is not backwards compatible, which can cause a problem for users who use this class.
This code will start throwing an exception if your change is accepted.

from airflow.contrib.operators.awsbatch_operator import AWSBatchOperator

Instead, it should display a deprecation message and work correctly in subsequent versions.
https://github.com/apache/airflow/pull/6455/files
https://github.com/apache/airflow/pull/6454/files

In the file https://github.com/apache/airflow/blob/master/tests/test_core_to_contrib.py we have tests that check whether previous import paths still work after moving the class to a new location. Your service has not been adapted to AIP-21, so it has not been added yet, but if you are moving files then you should move the files to a good place. The file cannot be moved to the wrong/temporary place, because migration requires the user's intervention and will be very confused if in each version he will have to change the import path.

Can you move also change location of tets?
https://github.com/apache/airflow/blob/master/tests/contrib/operators/test_awsbatch_operator.py

[dazza-codes]

Marking this PR as WIP to bring it into line with

• https://github.com/apache/airflow/blob/master/UPDATING.md#migration-of-aws-components
• https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-21%3A+Changes+in+import+paths

At the time of writing this comment, there is ongoing email threads about naming conventions and the location of the new provider packages. Some comments in those emails are proposing some changes that are inconsistent with the current master branch (or at least the UPDATING.md doc), so it's not entirely clear where to go with this PR to align it with AIP-21 exactly. IMO, this PR stands on it's merits as a stepping stone toward AIP-21, but the additional work to align it could be done here. I don't understand why it's been overlooked so far in other PRs for AIP-21.

[dazza-codes]

The latest commit will be squashed when it's working. The last piece is to get the core-to-contrib tests all passing, but I'm not understanding the failure for:

FAILED tests/test_core_to_contrib.py::TestMovingCoreToContrib::test_is_class_deprecated_7_airflow_providers_amazon_aws_operators_batch_AwsBatchOperator
- AssertionError: DeprecationWarning not triggered

The deprecation warning is coded for the module, but the class name is also changing from AWSBatchOperator to AwsBatchOperator and maybe that it throwing off the parameterized test somehow? The level of abstraction in the parameterized test is bending my mind a bit too far to immediately grok the problem.

Also, need to update doc links.

[dazza-codes]

Resolved WIP status and review requests to conform with AIP-21 (add the AIP-21 tag to the PR).

[dazza-codes]

The CI failed only for one "platform" and it's not related to this PR, i.e. failures were mostly cassandra:

SKIPPED [1] tests/sensors/test_sql_sensor.py:72: this is a postgres test
SKIPPED [1] tests/www/test_views.py:1817: flaky when run on mysql
FAILED tests/providers/apache/cassandra/hooks/test_cassandra_hook.py::TestCassandraHook::test_get_conn
FAILED tests/providers/apache/cassandra/hooks/test_cassandra_hook.py::TestCassandraHook::test_get_lb_policy
FAILED tests/providers/apache/cassandra/hooks/test_cassandra_hook.py::TestCassandraHook::test_record_exists_with_keyspace_from_cql
FAILED tests/providers/apache/cassandra/hooks/test_cassandra_hook.py::TestCassandraHook::test_record_exists_with_keyspace_from_session
FAILED tests/providers/apache/cassandra/hooks/test_cassandra_hook.py::TestCassandraHook::test_table_exists_with_keyspace_from_cql
FAILED tests/providers/apache/cassandra/hooks/test_cassandra_hook.py::TestCassandraHook::test_table_exists_with_keyspace_from_session
FAILED tests/providers/apache/cassandra/sensors/test_cassandra_sensor.py::TestCassandraRecordSensor::test_poke
FAILED tests/providers/apache/cassandra/sensors/test_cassandra_sensor.py::TestCassandraTableSensor::test_poke
==== 8 failed, 4037 passed, 115 skipped, 125 warnings in 1190.70s (0:19:50) ====

All I can do for that is hit a travis-retry button. Actually, I don't have travis permissions to retry that particular test plaform for Tests mysql python 3.7.

[dazza-codes]

I'll try to rebase and resolve conflicts after #6765 was merged (thanks).

[dazza-codes]

Rebased and pushed the AIP-21 changes.

• the git diff is not tracking it like a pure file move and might be losing some git history details, so if that's important for git-blame and other tracking, this might need to be split up into several commits for (a) a pure git-mv and (b) subsequent changes (ping me if this needs to happen, otherwise carry on regardless)

@mik-laj - it's ready for another round of review and please note that additional features are going to be stacked on this PR.