You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "Adaverse (via GitHub)" <gi...@apache.org> on 2023/07/24 18:47:00 UTC
[GitHub] [airflow] Adaverse opened a new pull request, #32810: Renames `db upgrade` with `db sync` and adds `create-default-connections`
Adaverse opened a new pull request, #32810:
URL: https://github.com/apache/airflow/pull/32810
Closes: #32420
Related: #32048
- [X] Renamed `db upgrade` with `db sync` and deprecated the former
- [X] Deprecated `db init` and added `create-default-connections` to create the default connection
- [X] Added test for the above change
- [ ] Document updation ongoing
This PR is part of PRs where we target to move the process of configuring default connections out of core Airflow to there respective providers.
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
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](https://github.com/apache/airflow/tree/main/newsfragments).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a diff in pull request #32810: Rename `db upgrade` to `db migrate` and add `connections create-default-connections`
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Adaverse commented on a diff in pull request #32810: Rename `db upgrade` to `db migrate` and add `connections create-default-connections`
Posted by "Adaverse (via GitHub)" <gi...@apache.org>.
Adaverse commented on code in PR #32810:
URL: https://github.com/apache/airflow/pull/32810#discussion_r1278289415
##########
docs/apache-airflow/administration-and-deployment/dag-serialization.rst:
##########
@@ -88,7 +88,7 @@ Add the following settings in ``airflow.cfg``:
* ``compress_serialized_dags``: This option controls whether to compress the Serialized DAG to the Database.
It is useful when there are very large DAGs in your cluster. When ``True``, this will disable the DAG dependencies view.
-If you are updating Airflow from <1.10.7, please do not forget to run ``airflow db upgrade``.
+If you are updating Airflow from <1.10.7, please do not forget to run ``airflow db migrate``.
Review Comment:
Oops, my bad. There was another place where it wouldn't have. Updated it. Thanks
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Adaverse commented on a diff in pull request #32810: Rename `db upgrade` to `db migrate` and add `connections create-default-connections`
Posted by "Adaverse (via GitHub)" <gi...@apache.org>.
Adaverse commented on code in PR #32810:
URL: https://github.com/apache/airflow/pull/32810#discussion_r1279219627
##########
docs/apache-airflow/installation/upgrading.rst:
##########
@@ -21,8 +21,12 @@ 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``
+Newer Airflow versions can contain database migrations so you must run ``airflow db migrate``
+<<<<<<< HEAD
Review Comment:
Removed!
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Adaverse commented on pull request #32810: Rename `db upgrade` to `db sync` and add `create-default-connections`
Posted by "Adaverse (via GitHub)" <gi...@apache.org>.
Adaverse commented on PR #32810:
URL: https://github.com/apache/airflow/pull/32810#issuecomment-1654614293
> I also think there was a way `help=argparse.SUPPRESS`
> I think is the way to hide the option from `--help` output and also not to show it in our documentaiton - which is basically what we want. old users who are used to it will stil be able to use it, new users won't because they won't discover it.
Yes we can hide arguments using `argparse.SUPPRESS` but it does not work when applied on `sub_parser`. It can be hidden by emitting help parameter from the corresponding sub commands as explained [here](https://bugs.python.org/issue22848). But this way it appears in the doc with text undocumented below the command.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Adaverse commented on a diff in pull request #32810: Rename `db upgrade` to `db migrate` and add `connections create-default-connections`
Posted by "Adaverse (via GitHub)" <gi...@apache.org>.
Adaverse commented on code in PR #32810:
URL: https://github.com/apache/airflow/pull/32810#discussion_r1279616657
##########
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:
Yes, you are right. Let me make the change.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #32810: Rename `db upgrade` to `db sync` and add `create-default-connections`
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on PR #32810:
URL: https://github.com/apache/airflow/pull/32810#issuecomment-1655047007
Was there a discussion to use `migrate` instead? (Not that I object to `migrate`, just curious about the sudden change.)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Adaverse commented on a diff in pull request #32810: Rename `db upgrade` to `db migrate` and add `connections create-default-connections`
Posted by "Adaverse (via GitHub)" <gi...@apache.org>.
Adaverse commented on code in PR #32810:
URL: https://github.com/apache/airflow/pull/32810#discussion_r1279728858
##########
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:
Removed the `HIDE_SUBCOMMAND` and added `hide` keyword in the `ActionCommand`. It's much simpler now. Thanks!
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Adaverse commented on a diff in pull request #32810: Rename `db upgrade` to `db sync` and add `create-default-connections`
Posted by "Adaverse (via GitHub)" <gi...@apache.org>.
Adaverse commented on code in PR #32810:
URL: https://github.com/apache/airflow/pull/32810#discussion_r1277791356
##########
airflow/cli/cli_config.py:
##########
@@ -1721,6 +1747,12 @@ class GroupCommand(NamedTuple):
func=lazy_load_command("airflow.cli.commands.db_command.drop_archived"),
args=(ARG_DB_TABLES, ARG_YES),
),
+ ActionCommand(
Review Comment:
Yes, on it!
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Adaverse commented on a diff in pull request #32810: Rename `db upgrade` to `db migrate` and add `connections create-default-connections`
Posted by "Adaverse (via GitHub)" <gi...@apache.org>.
Adaverse commented on code in PR #32810:
URL: https://github.com/apache/airflow/pull/32810#discussion_r1279807909
##########
airflow/cli/cli_parser.py:
##########
@@ -137,9 +137,12 @@ def get_long_option(arg: Arg):
def _add_command(subparsers: argparse._SubParsersAction, sub: CLICommand) -> None:
- sub_proc = subparsers.add_parser(
- sub.name, help=sub.help, description=sub.description or sub.help, epilog=sub.epilog
- )
+ if isinstance(sub, ActionCommand) and sub.hide:
+ sub_proc = subparsers.add_parser(sub.name, epilog=sub.epilog)
Review Comment:
Nope, they do get all the args (only help is hidden) --
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32810: Rename `db upgrade` to `db migrate` and add `connections create-default-connections`
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32810:
URL: https://github.com/apache/airflow/pull/32810#discussion_r1279212932
##########
docs/docker-stack/entrypoint.rst:
##########
@@ -315,8 +315,8 @@ either as maintenance operations on the database or should be embedded in the cu
Upgrading Airflow DB
....................
-If you set :envvar:`_AIRFLOW_DB_UPGRADE` variable to a non-empty value, the entrypoint will run
-the ``airflow db upgrade`` command right after verifying the connection. You can also use this
+If you set :envvar:`_AIRFLOW_DB_MIGRATE` variable to a non-empty value, the entrypoint will run
Review Comment:
Good spot!
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #32810: Rename `db upgrade` to `db migrate` and add `connections create-default-connections`
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #32810:
URL: https://github.com/apache/airflow/pull/32810#discussion_r1278662251
##########
scripts/docker/entrypoint_prod.sh:
##########
@@ -283,8 +283,8 @@ if [[ "${CONNECTION_CHECK_MAX_COUNT}" -gt "0" ]]; then
wait_for_airflow_db
fi
-if [[ -n "${_AIRFLOW_DB_UPGRADE=}" ]] ; then
- upgrade_db
+if [[ -n "${_AIRFLOW_DB_UPGRADE=}" ]] || [[ -n "${_AIRFLOW_DB_MIGRATE=}" ]] ; then
Review Comment:
Should we add a warning if the `UPGRADE` environment variable is used?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a diff in pull request #32810: Rename `db upgrade` to `db migrate` and add `connections create-default-connections`
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #32810:
URL: https://github.com/apache/airflow/pull/32810#discussion_r1279580534
##########
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"):
```
Normally the expected warning is a positional arg. That coupled with the warning message changes above might allow this to land on 1 line?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Adaverse commented on pull request #32810: Rename `db upgrade` with `db sync` and add `create-default-connections`
Posted by "Adaverse (via GitHub)" <gi...@apache.org>.
Adaverse commented on PR #32810:
URL: https://github.com/apache/airflow/pull/32810#issuecomment-1648431370
cc - @potiuk @eladkal
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32810: Rename `db upgrade` to `db sync` and add `create-default-connections`
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32810:
URL: https://github.com/apache/airflow/pull/32810#discussion_r1277744369
##########
airflow/cli/cli_config.py:
##########
@@ -1721,6 +1747,12 @@ class GroupCommand(NamedTuple):
func=lazy_load_command("airflow.cli.commands.db_command.drop_archived"),
args=(ARG_DB_TABLES, ARG_YES),
),
+ ActionCommand(
Review Comment:
Should we already move it to the connections command?
##########
airflow/cli/cli_config.py:
##########
@@ -1721,6 +1747,12 @@ class GroupCommand(NamedTuple):
func=lazy_load_command("airflow.cli.commands.db_command.drop_archived"),
args=(ARG_DB_TABLES, ARG_YES),
),
+ ActionCommand(
Review Comment:
Should we already move it to the connections command in this PR?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Adaverse commented on a diff in pull request #32810: Rename `db upgrade` to `db migrate` and add `connections create-default-connections`
Posted by "Adaverse (via GitHub)" <gi...@apache.org>.
Adaverse commented on code in PR #32810:
URL: https://github.com/apache/airflow/pull/32810#discussion_r1278133857
##########
docs/docker-stack/entrypoint.rst:
##########
@@ -315,8 +315,8 @@ either as maintenance operations on the database or should be embedded in the cu
Upgrading Airflow DB
....................
-If you set :envvar:`_AIRFLOW_DB_UPGRADE` variable to a non-empty value, the entrypoint will run
-the ``airflow db upgrade`` command right after verifying the connection. You can also use this
+If you set :envvar:`_AIRFLOW_DB_MIGRATE` variable to a non-empty value, the entrypoint will run
Review Comment:
Renamed the env var as well for consistency!
##########
scripts/docker/entrypoint_prod.sh:
##########
@@ -283,8 +283,8 @@ if [[ "${CONNECTION_CHECK_MAX_COUNT}" -gt "0" ]]; then
wait_for_airflow_db
fi
-if [[ -n "${_AIRFLOW_DB_UPGRADE=}" ]] ; then
- upgrade_db
+if [[ -n "${_AIRFLOW_DB_UPGRADE=}" ]] || [[ -n "${_AIRFLOW_DB_MIGRATE=}" ]] ; then
Review Comment:
Made this change so that its backward compatible
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Adaverse commented on a diff in pull request #32810: Rename `db upgrade` to `db migrate` and add `connections create-default-connections`
Posted by "Adaverse (via GitHub)" <gi...@apache.org>.
Adaverse commented on code in PR #32810:
URL: https://github.com/apache/airflow/pull/32810#discussion_r1279720576
##########
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:
Indeed it does!
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Adaverse commented on a diff in pull request #32810: Rename `db upgrade` to `db migrate` and add `connections create-default-connections`
Posted by "Adaverse (via GitHub)" <gi...@apache.org>.
Adaverse commented on code in PR #32810:
URL: https://github.com/apache/airflow/pull/32810#discussion_r1278760195
##########
scripts/docker/entrypoint_prod.sh:
##########
@@ -283,8 +283,8 @@ if [[ "${CONNECTION_CHECK_MAX_COUNT}" -gt "0" ]]; then
wait_for_airflow_db
fi
-if [[ -n "${_AIRFLOW_DB_UPGRADE=}" ]] ; then
- upgrade_db
+if [[ -n "${_AIRFLOW_DB_UPGRADE=}" ]] || [[ -n "${_AIRFLOW_DB_MIGRATE=}" ]] ; then
Review Comment:
Yes. Added a warning.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Adaverse commented on a diff in pull request #32810: Rename `db upgrade` to `db migrate` and add `connections create-default-connections`
Posted by "Adaverse (via GitHub)" <gi...@apache.org>.
Adaverse commented on code in PR #32810:
URL: https://github.com/apache/airflow/pull/32810#discussion_r1278001774
##########
airflow/cli/cli_config.py:
##########
@@ -1721,6 +1747,12 @@ class GroupCommand(NamedTuple):
func=lazy_load_command("airflow.cli.commands.db_command.drop_archived"),
args=(ARG_DB_TABLES, ARG_YES),
),
+ ActionCommand(
Review Comment:
There are few places where `initdb` from `airflow.utils.db` is being called that creates default connection ex -
https://github.com/apache/airflow/blob/f91c003092e0e2c0e5b9a5073352b023a51a887f/airflow/utils/db.py#L1669-L1670
https://github.com/apache/airflow/blob/f91c003092e0e2c0e5b9a5073352b023a51a887f/airflow/utils/db.py#L845-L848
Shall we replace these now with `upgradedb` or maybe in later commits when we would have added the desired functionality to `create_default_connections`?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32810: Rename `db upgrade` to `db migrate` and add `connections create-default-connections`
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32810:
URL: https://github.com/apache/airflow/pull/32810#discussion_r1278078139
##########
airflow/cli/cli_config.py:
##########
@@ -1721,6 +1747,12 @@ class GroupCommand(NamedTuple):
func=lazy_load_command("airflow.cli.commands.db_command.drop_archived"),
args=(ARG_DB_TABLES, ARG_YES),
),
+ ActionCommand(
Review Comment:
> I hope it is fine now to replace every instance of db init and db upgrade with db migrate in the doc?
yes
> Shall we replace these now with upgradedb or maybe in later commits when we would have added the desired functionality to create_default_connections?
should be `migrate` in both cases I think. That was the intention in both cases, I think
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Adaverse commented on a diff in pull request #32810: Rename `db upgrade` to `db migrate` and add `connections create-default-connections`
Posted by "Adaverse (via GitHub)" <gi...@apache.org>.
Adaverse commented on code in PR #32810:
URL: https://github.com/apache/airflow/pull/32810#discussion_r1278102597
##########
airflow/cli/cli_config.py:
##########
@@ -1721,6 +1747,12 @@ class GroupCommand(NamedTuple):
func=lazy_load_command("airflow.cli.commands.db_command.drop_archived"),
args=(ARG_DB_TABLES, ARG_YES),
),
+ ActionCommand(
Review Comment:
> should be `migrate` in both cases I think.
Currently, we have just renamed the command and the function names in `db_commands.py`. I think its best to take the renaming of `airflow.utils.db.upgradedb` to be taken in another PR since it is used at lots of other places and quite some refactoring is required for above change.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Adaverse commented on pull request #32810: Rename `db upgrade` to `db sync` and add `create-default-connections`
Posted by "Adaverse (via GitHub)" <gi...@apache.org>.
Adaverse commented on PR #32810:
URL: https://github.com/apache/airflow/pull/32810#issuecomment-1655066211
> Was there a discussion to use `migrate` instead? (Not that I object to `migrate`, just curious about the sudden change.)
Have created an announcement in the dev channel asking what everybody thinks - https://lists.apache.org/thread/3njd9l8fpcshnkk61t74orntp13w4vjo
The above change is not yet final in any way. Depends on collective agreement.
Do I also need to cc some other channels?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Adaverse commented on a diff in pull request #32810: Rename `db upgrade` to `db migrate` and add `connections create-default-connections`
Posted by "Adaverse (via GitHub)" <gi...@apache.org>.
Adaverse commented on code in PR #32810:
URL: https://github.com/apache/airflow/pull/32810#discussion_r1279671156
##########
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:
Noted!
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32810: Rename `db upgrade` to `db migrate` and add `connections create-default-connections`
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32810:
URL: https://github.com/apache/airflow/pull/32810#discussion_r1278151967
##########
docs/apache-airflow/administration-and-deployment/dag-serialization.rst:
##########
@@ -88,7 +88,7 @@ Add the following settings in ``airflow.cfg``:
* ``compress_serialized_dags``: This option controls whether to compress the Serialized DAG to the Database.
It is useful when there are very large DAGs in your cluster. When ``True``, this will disable the DAG dependencies view.
-If you are updating Airflow from <1.10.7, please do not forget to run ``airflow db upgrade``.
+If you are updating Airflow from <1.10.7, please do not forget to run ``airflow db migrate``.
Review Comment:
I think this line can be removed - it refers to really, really old stuff and there "migrate" wouldn't work.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Adaverse commented on a diff in pull request #32810: Rename `db upgrade` to `db migrate` and add `connections create-default-connections`
Posted by "Adaverse (via GitHub)" <gi...@apache.org>.
Adaverse commented on code in PR #32810:
URL: https://github.com/apache/airflow/pull/32810#discussion_r1278289460
##########
docs/apache-airflow/administration-and-deployment/production-deployment.rst:
##########
@@ -43,14 +43,9 @@ 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.
-
-.. 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.
+``migrate`` keeps track of migrations already applied, so it's safe to run as often as you need.
Review Comment:
Added notes in a few important places for context.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #32810: Rename `db upgrade` to `db migrate` and add `connections create-default-connections`
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #32810:
URL: https://github.com/apache/airflow/pull/32810
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a diff in pull request #32810: Rename `db upgrade` to `db migrate` and add `connections create-default-connections`
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #32810:
URL: https://github.com/apache/airflow/pull/32810#discussion_r1279791403
##########
airflow/cli/cli_parser.py:
##########
@@ -137,9 +137,12 @@ def get_long_option(arg: Arg):
def _add_command(subparsers: argparse._SubParsersAction, sub: CLICommand) -> None:
- sub_proc = subparsers.add_parser(
- sub.name, help=sub.help, description=sub.description or sub.help, epilog=sub.epilog
- )
+ if isinstance(sub, ActionCommand) and sub.hide:
+ sub_proc = subparsers.add_parser(sub.name, epilog=sub.epilog)
Review Comment:
How does this work if someone ran `airflow db upgrade --help`? Do they get nothing?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32810: Rename `db upgrade` to `db migrate` and add `connections create-default-connections`
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32810:
URL: https://github.com/apache/airflow/pull/32810#discussion_r1279212138
##########
docs/apache-airflow/installation/upgrading.rst:
##########
@@ -21,8 +21,12 @@ 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``
+Newer Airflow versions can contain database migrations so you must run ``airflow db migrate``
+<<<<<<< HEAD
Review Comment:
conflicts marker here to remove :)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #32810: Rename `db upgrade` to `db sync` and add `create-default-connections`
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32810:
URL: https://github.com/apache/airflow/pull/32810#issuecomment-1650589877
Two things:
* I would add a "significant" newsfragment describing it. I also think there was a way
* `help=argparse.SUPPRESS` I think is the way to hide the option from --help output and also not to show it in our documentaiton - which is basically what we want. old users who are used to it will stil be able to use it, new users won't because they won't discover it.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #32810: Rename `db upgrade` to `db sync` and add `create-default-connections`
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on PR #32810:
URL: https://github.com/apache/airflow/pull/32810#issuecomment-1655069235
No this is good, I just wondered if it’s based on a decision made somewhere I’m not aware of.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Adaverse commented on a diff in pull request #32810: Rename `db upgrade` to `db sync` and add `create-default-connections`
Posted by "Adaverse (via GitHub)" <gi...@apache.org>.
Adaverse commented on code in PR #32810:
URL: https://github.com/apache/airflow/pull/32810#discussion_r1277948154
##########
airflow/cli/cli_config.py:
##########
@@ -1721,6 +1747,12 @@ class GroupCommand(NamedTuple):
func=lazy_load_command("airflow.cli.commands.db_command.drop_archived"),
args=(ARG_DB_TABLES, ARG_YES),
),
+ ActionCommand(
Review Comment:
I hope it is fine now to replace every instance of `db init` and `db upgrade` with `db migrate` in the doc?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32810: Rename `db upgrade` to `db migrate` and add `connections create-default-connections`
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32810:
URL: https://github.com/apache/airflow/pull/32810#discussion_r1278152738
##########
docs/apache-airflow/administration-and-deployment/production-deployment.rst:
##########
@@ -43,14 +43,9 @@ 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.
-
-.. 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.
+``migrate`` keeps track of migrations already applied, so it's safe to run as often as you need.
Review Comment:
It would be great to add in ` .. note` here that formerly it was `upgrade`. (before 2.7) some people will look at latest version of the docs and try with older airflow version - note here and in other places of the .rst docs might be useful.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org