jedcunningham commented on code in PR #32810:
URL: https://github.com/apache/airflow/pull/32810#discussion_r1279533936
##########
airflow/cli/commands/db_command.py:
##########
@@ -39,6 +40,11 @@
@providers_configuration_loaded
def initdb(args):
"""Initializes the metadata database."""
+ warnings.warn(
+ "ActionCommand `init` is deprecated. Use `migrate` instead to migrate
the db and/or "
Review Comment:
```suggestion
"`db init` is deprecated. Use `db migrate` instead to migrate the
db and/or "
```
This is a better message to show to users.
##########
docs/apache-airflow/administration-and-deployment/production-deployment.rst:
##########
@@ -43,14 +43,14 @@ Once that is done, you can run -
.. code-block:: bash
- airflow db upgrade
+ airflow db migrate
-``upgrade`` keeps track of migrations already applied, so it's safe to run as
often as you need.
+``migrate`` keeps track of migrations already applied, so it's safe to run as
often as you need.
.. note::
- Do not use ``airflow db init`` as it can create a lot of default
connections, charts, etc. which are not
- required in production DB.
+ Prior to Airflow version 2.7.0, ``airflow db upgrade`` was used to apply
migrations.
+ But since 2.7.0, it has been deprecated in favor of ``airflow db migrate``
Review Comment:
```suggestion
Prior to Airflow version 2.7.0, ``airflow db upgrade`` was used to apply
migrations,
however, it has been deprecated in favor of ``airflow db migrate``.
```
##########
airflow/models/crypto.py:
##########
@@ -42,7 +42,7 @@ class NullFernet:
The purpose of this is to make the rest of the code not have to know the
difference, and to only display the message once, not 20 times when
- `airflow db init` is ran.
+ `airflow db migrate` is runs.
Review Comment:
```suggestion
`airflow db migrate` is run.
```
##########
docs/apache-airflow/installation/setting-up-the-database.rst:
##########
@@ -22,11 +22,14 @@ Apache Airflow™ requires a database. If you're just
experimenting and learning
default SQLite option. If you don't want to use SQLite, then take a look at
:doc:`/howto/set-up-database` to setup a different database.
-Usually, you need to run ``airflow db upgrade`` in order to create the
database schema that Airflow can use.
+Usually, you need to run ``airflow db migrate`` in order to create the
database schema if it does not exist
+or migrate to the latest version if it does.
+
+.. note::
+
+ Prior to Airflow version 2.7.0, ``airflow db init`` was used to create the
db and ``airflow db upgrade`` to migrate it.
+ Now it has been replaced by one command ``airflow db migrate`` that does
both and above two commands are now deprecated.
Review Comment:
```suggestion
Prior to Airflow version 2.7.0, ``airflow db upgrade`` was used to apply
migrations,
however, it has been deprecated in favor of ``airflow db migrate``.
```
Upgrade already did both. Let's keep this note simple.
##########
airflow/cli/cli_parser.py:
##########
@@ -65,6 +65,9 @@
ALL_COMMANDS_DICT: dict[str, CLICommand] = {sp.name: sp for sp in
airflow_commands}
+# Hides the subcommands (list) for the given command (key)
+HIDE_SUBCOMMAND = {"db": ["init", "upgrade"]}
Review Comment:
Can we build support for this in ActionCommand instead, maybe a "hide" or
"deprecated"? Keeping it where the command is defined seems like it'd be more
clear and less likely to be overlooked later.
##########
docs/apache-airflow/installation/upgrading.rst:
##########
@@ -41,7 +41,7 @@ When you need to upgrade
========================
If you have a custom deployment based on virtualenv or Docker Containers, you
usually need to run
-the DB upgrade manually as part of the upgrade process.
+the DB migrate manually as part of the migration process.
Review Comment:
```suggestion
the DB migrate manually as part of the upgrade process.
```
##########
airflow/cli/commands/db_command.py:
##########
@@ -96,7 +108,7 @@ def upgradedb(args):
reserialize_dags=args.reserialize_dags,
)
if not args.show_sql_only:
- print("Upgrades done")
+ print("Database syncing done!")
Review Comment:
```suggestion
print("Database migrating done!")
```
##########
docs/apache-airflow/installation/upgrading.rst:
##########
@@ -21,8 +21,8 @@ Upgrading Airflow™ to a newer version
Why you need to upgrade
=======================
-Newer Airflow versions can contain database migrations so you must run
``airflow db upgrade``
-to upgrade your database with the schema changes in the Airflow version you
are upgrading to.
+Newer Airflow versions can contain database migrations so you must run
``airflow db migrate``
+to migrate your database with the schema changes in the Airflow version you
are migrating to.
Review Comment:
```suggestion
to migrate your database with the schema changes in the Airflow version you
are upgrading to.
```
##########
tests/cli/commands/test_db_command.py:
##########
@@ -37,8 +37,11 @@ def setup_class(cls):
@mock.patch("airflow.cli.commands.db_command.db.initdb")
def test_cli_initdb(self, mock_initdb):
- db_command.initdb(self.parser.parse_args(["db", "init"]))
-
+ with pytest.warns(
+ expected_warning=DeprecationWarning, match="ActionCommand `init`
is deprecated"
+ ) as warning_record:
Review Comment:
```suggestion
with pytest.warns(DeprecationWarning, match="`init` is deprecated")
as warning_record:
```
Normally the expected warning is a positional arg. That coupled with the
warning message changes above might allow this to land on 1 line?
##########
docs/apache-airflow/installation/setting-up-the-database.rst:
##########
@@ -22,11 +22,14 @@ Apache Airflow™ requires a database. If you're just
experimenting and learning
default SQLite option. If you don't want to use SQLite, then take a look at
:doc:`/howto/set-up-database` to setup a different database.
-Usually, you need to run ``airflow db upgrade`` in order to create the
database schema that Airflow can use.
+Usually, you need to run ``airflow db migrate`` in order to create the
database schema if it does not exist
+or migrate to the latest version if it does.
Review Comment:
```suggestion
or migrate to the latest version if it does. You should make sure that
Airflow components are
not running while the database migration is being executed.
```
Let's not lose this part.
##########
airflow/cli/commands/db_command.py:
##########
@@ -53,10 +59,16 @@ def resetdb(args):
db.resetdb(skip_init=args.skip_init)
-@cli_utils.action_cli(check_db=False)
-@providers_configuration_loaded
def upgradedb(args):
"""Upgrades the metadata database."""
+ warnings.warn("ActionCommand `updgrade` is deprecated. Use `migrate`
instead.", DeprecationWarning)
Review Comment:
```suggestion
warnings.warn("`db updgrade` is deprecated. Use `db migrate` instead.",
DeprecationWarning)
```
##########
tests/cli/commands/test_db_command.py:
##########
@@ -37,8 +37,11 @@ def setup_class(cls):
@mock.patch("airflow.cli.commands.db_command.db.initdb")
def test_cli_initdb(self, mock_initdb):
- db_command.initdb(self.parser.parse_args(["db", "init"]))
-
+ with pytest.warns(
+ expected_warning=DeprecationWarning, match="ActionCommand `init`
is deprecated"
+ ) as warning_record:
+ db_command.initdb(self.parser.parse_args(["db", "init"]))
+ assert warning_record
Review Comment:
```suggestion
```
It'll fail already if a warning doesn't happen.
--
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]
