You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/12/01 15:07:35 UTC
[GitHub] [airflow] Nick-0723 opened a new pull request #12740: Fix the exception that the port is empty when using db shell
Nick-0723 opened a new pull request #12740:
URL: https://github.com/apache/airflow/pull/12740
<!--
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 existing issue, reference it using one of the following:
closes: #ISSUE
related: #12739
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
Apache Airflow version: 2.0.0b3
Kubernetes version (if you are using kubernetes) (use kubectl version):
Environment:
Cloud provider or hardware configuration:
OS (e.g. from /etc/os-release): Debian GNU/Linux 10 (buster)
Kernel (e.g. uname -a): Linux ace72a077a8c 4.19.76-linuxkit #1 SMP Tue May 26 11:42:35 UTC 2020 x86_64 GNU/Linux
Install tools: ./breeze --backend mysql --integration redis --db-reset
Others:
What happened: Encountered an error "mysql: [ERROR] mysql: Empty value for'port' specified." using airflow db shell in breeze。 An error will be encountered when the port is not set in sql_alchemy_conn. Like the default value "mysql://root@mysql/airflow?charset=utf8mb4"
What you expected to happen: The default port should be used in db_command
How to reproduce it: use './breeze --backend mysql --integration redis --db-reset' ,and then run 'airflow db shell'
ISSUE: #12739
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #12740: Fix the exception that the port is empty when using db shell
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #12740:
URL: https://github.com/apache/airflow/pull/12740
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #12740: Fix the exception that the port is empty when using db shell
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12740:
URL: https://github.com/apache/airflow/pull/12740#discussion_r535255244
##########
File path: airflow/cli/commands/db_command.py
##########
@@ -71,6 +71,7 @@ def shell(args):
database = {url.database}
"""
).strip()
+ print(content)
Review comment:
Just one thing -> can you remove that - seems like a debug statement
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Nick-0723 commented on a change in pull request #12740: Fix the exception that the port is empty when using db shell
Posted by GitBox <gi...@apache.org>.
Nick-0723 commented on a change in pull request #12740:
URL: https://github.com/apache/airflow/pull/12740#discussion_r535297026
##########
File path: airflow/cli/commands/db_command.py
##########
@@ -71,6 +71,7 @@ def shell(args):
database = {url.database}
"""
).strip()
+ print(content)
Review comment:
Yes， my mistake. Thank you very much for your guidance. It’s the first time I participated in contributing to an open source project
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #12740: Fix the exception that the port is empty when using db shell
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12740:
URL: https://github.com/apache/airflow/pull/12740#issuecomment-738146213
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest master or amend the last commit of the PR, and push it with --force-with-lease.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Nick-0723 commented on a change in pull request #12740: Fix the exception that the port is empty when using db shell
Posted by GitBox <gi...@apache.org>.
Nick-0723 commented on a change in pull request #12740:
URL: https://github.com/apache/airflow/pull/12740#discussion_r533833066
##########
File path: airflow/cli/commands/db_command.py
##########
@@ -67,7 +67,7 @@ def shell(args):
host = {url.host}
user = {url.username}
password = {url.password or ""}
- port = {url.port or ""}
Review comment:
Thanks for your guidance, I should think more comprehensively
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #12740: Fix the exception that the port is empty when using db shell
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #12740:
URL: https://github.com/apache/airflow/pull/12740#issuecomment-738192108
Awesome work, congrats on your first merged pull request!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #12740: Fix the exception that the port is empty when using db shell
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12740:
URL: https://github.com/apache/airflow/pull/12740#discussion_r533573759
##########
File path: tests/cli/commands/test_db_command.py
##########
@@ -56,13 +56,13 @@ def test_cli_upgradedb(self, mock_upgradedb):
@mock.patch("airflow.cli.commands.db_command.execute_interactive")
@mock.patch("airflow.cli.commands.db_command.NamedTemporaryFile")
- @mock.patch("airflow.cli.commands.db_command.settings.engine.url", make_url("mysql://root@mysql/airflow"))
Review comment:
Why don't we parameterise the tests and add with/without port mysql/postgres?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #12740: Fix the exception that the port is empty when using db shell
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #12740:
URL: https://github.com/apache/airflow/pull/12740#issuecomment-738043522
Cool! 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #12740: Fix the exception that the port is empty when using db shell
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12740:
URL: https://github.com/apache/airflow/pull/12740#discussion_r533572104
##########
File path: airflow/cli/commands/db_command.py
##########
@@ -67,7 +67,7 @@ def shell(args):
host = {url.host}
user = {url.username}
password = {url.password or ""}
- port = {url.port or ""}
Review comment:
I think it should depend on the database: Mysql = 3306, Postgres = 5432 -> so possibly we should add it for posgres few lines below.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #12740: Fix the exception that the port is empty when using db shell
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #12740:
URL: https://github.com/apache/airflow/pull/12740#issuecomment-736612303
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, pylint and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/master/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/master/docs/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/master/BREEZE.rst) for testing locally, it’s a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #12740: Fix the exception that the port is empty when using db shell
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12740:
URL: https://github.com/apache/airflow/pull/12740#discussion_r533572104
##########
File path: airflow/cli/commands/db_command.py
##########
@@ -67,7 +67,7 @@ def shell(args):
host = {url.host}
user = {url.username}
password = {url.password or ""}
- port = {url.port or ""}
Review comment:
I think it should depend on the database: Mysql = 3306, Postgres = 5432
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #12740: Fix the exception that the port is empty when using db shell
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12740:
URL: https://github.com/apache/airflow/pull/12740#discussion_r533572104
##########
File path: airflow/cli/commands/db_command.py
##########
@@ -67,7 +67,7 @@ def shell(args):
host = {url.host}
user = {url.username}
password = {url.password or ""}
- port = {url.port or ""}
Review comment:
I think it should depend on the database: Mysql = 3306, Postgres = 5432 -> so possibly we should add it for postgres few lines below.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #12740: Fix the exception that the port is empty when using db shell
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12740:
URL: https://github.com/apache/airflow/pull/12740#discussion_r535389739
##########
File path: airflow/cli/commands/db_command.py
##########
@@ -71,6 +71,7 @@ def shell(args):
database = {url.database}
"""
).strip()
+ print(content)
Review comment:
Happens to everyone. Nice one for the first contribution. We hope to see you more frequently around!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org