Create UsingSQLFromBaseHookRule to ease upgrade to Airflow 2.0

[turbaszek]

This issue is part of #8765

Rule

Create UsingSQLFromBaseHookRule which corresponds to

Remove SQL support in base_hook

entry in UPDATING.md. This rule should allow users to check if their current configuration needs any adjusting
before migration to Airflow 2.0.

How to guide

To implement a new rule, create a class that inherits from airflow.upgrade.rules.base_rule.BaseRule.
It will be auto-registered and used by airflow upgrade-check command. The custom rule class has to have title,
description properties and should implement check method which returns a list of error messages in case of
incompatibility.

For example:

airflow/airflow/upgrade/rules/conn_type_is_not_nullable.py

Lines 25 to 42 in ea36166

	class ConnTypeIsNotNullableRule(BaseRule):
	title = "Connection.conn_type is not nullable"
	description = """\
	The `conn_type` column in the `connection` table must contain content. Previously, this rule was \
	enforced by application logic, but was not enforced by the database schema.
	If you made any modifications to the table directly, make sure you don't have null in the conn_type column.\
	"""
	@provide_session
	def check(self, session=None):
	invalid_connections = session.query(Connection).filter(Connection.conn_type.is_(None))
	return (
	'Connection<id={}", conn_id={}> have empty conn_type field.'.format(conn.id, conn.conn_id)
	for conn in invalid_connections
	)

Remember to open the PR against v1-10-test branch.

[pcandoalmeida]

Hi @turbaszek 👋🏼 could I work on this? I think I'm almost done with the GCP account service key rule.

[mik-laj]

@pcandoalmeida I assigned you to this ticket.

[pcandoalmeida]

Hi @turbaszek 👋🏼 I wanted to check that I'm understanding this rule correctly. Here, no configuration is set; either, users will be extending the BaseHook class or using the removed class methods directly. Would we parse custom operators for this check or would we perhaps return a warning?

[turbaszek]

Here, no configuration is set; either, users will be extending the BaseHook class or using the removed class methods directly.

Yes, it's all about using the removed methods.

Would we parse custom operators for this check or would we perhaps return a warning?

I'm personally in favor of checking if users use any custom operator - if yes we should inform them about this change. I'm afraid that checking this with 100% of accuracy may be really hard. @mik-laj @kaxil what is your opinion?

[pcandoalmeida]

I'm afraid that checking this with 100% of accuracy may be really hard

Agree. If it proves too tricky, could we return a blanket WARNING as a workaround idea?

[turbaszek]

I'm afraid that checking this with 100% of accuracy may be really hard

Agree. If it proves too tricky, could we return a blanket WARNING as a workaround idea?

I think yes. We may consider introducing in checker a new type "WARNING" (next to "SUCCESS" and "FAIL").

[pcandoalmeida]

Hey @turbaszek is the above work something I could do perhaps? If it's straightforward, I could perhaps implement it and then try and finish this rule 😃

[turbaszek]

@pcandoalmeida feel free to give it a shot 🚀

[dimberman]

@turbaszek I think I have a solution. We can basically check any hook that inherits from base_hook and ensure that they don't use the function. Do you think this would work?

[turbaszek]

@turbaszek I think I have a solution. We can basically check any hook that inherits from base_hook and ensure that they don't use the function. Do you think this would work?

We can, but how? We should not only check custom hooks imported in DAGs but also custom operators that may use BaseHook somewhere.

[turbaszek]

Done in #12730