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/12 21:14:25 UTC
[GitHub] [airflow] turbaszek opened a new pull request #13036: Refactor plugins command output using AirflowConsole
turbaszek opened a new pull request #13036:
URL: https://github.com/apache/airflow/pull/13036
This PR refactors the airflow plugins command to be compatible with
'output' parameter which allows users to get output in form of table,
json or yaml.
<!--
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: #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/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+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 [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
----------------------------------------------------------------
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] XD-DENG commented on a change in pull request #13036: Refactor plugins command output using AirflowConsole
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on a change in pull request #13036:
URL: https://github.com/apache/airflow/pull/13036#discussion_r541783391
##########
File path: airflow/cli/commands/plugins_command.py
##########
@@ -15,17 +15,14 @@
# specific language governing permissions and limitations
# under the License.
import inspect
-from typing import Any, List, Optional, Union
-
-from rich.console import Console
+from typing import Any, Dict, List, Union
from airflow import plugins_manager
-from airflow.cli.simple_table import SimpleTable
-from airflow.configuration import conf
+from airflow.cli.simple_table import AirflowConsole
from airflow.plugins_manager import PluginsDirectorySource
+from airflow.utils.cli import suppress_logs_and_warning
# list to maintain the order of items.
Review comment:
This line of comment can be removed if we confirm `PLUGINS_MANAGER_ATTRIBUTES_TO_DUMP` can be removed
##########
File path: airflow/cli/commands/plugins_command.py
##########
@@ -15,17 +15,14 @@
# specific language governing permissions and limitations
# under the License.
import inspect
-from typing import Any, List, Optional, Union
-
-from rich.console import Console
+from typing import Any, Dict, List, Union
from airflow import plugins_manager
-from airflow.cli.simple_table import SimpleTable
-from airflow.configuration import conf
+from airflow.cli.simple_table import AirflowConsole
from airflow.plugins_manager import PluginsDirectorySource
+from airflow.utils.cli import suppress_logs_and_warning
# list to maintain the order of items.
-from airflow.utils.cli import suppress_logs_and_warning
PLUGINS_MANAGER_ATTRIBUTES_TO_DUMP = [
Review comment:
This can be removed now 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #13036: Refactor plugins command output using AirflowConsole
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13036:
URL: https://github.com/apache/airflow/pull/13036#issuecomment-744019859
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] turbaszek commented on a change in pull request #13036: Refactor plugins command output using AirflowConsole
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #13036:
URL: https://github.com/apache/airflow/pull/13036#discussion_r541938348
##########
File path: airflow/cli/commands/plugins_command.py
##########
@@ -78,33 +75,17 @@ def dump_plugins(args):
print("No plugins loaded")
return
- console = Console()
- console.print("[bold yellow]SUMMARY:[/bold yellow]")
- console.print(
- f"[bold green]Plugins directory[/bold green]: {conf.get('core', 'plugins_folder')}\n", highlight=False
- )
- console.print(
- f"[bold green]Loaded plugins[/bold green]: {len(plugins_manager.plugins)}\n", highlight=False
- )
+ plugins_info: List[Dict[str, str]] = []
+ for plugin in plugins_manager.plugins:
+ info = {"name": plugin.name}
+ info.update({n: getattr(plugin, n) for n in PLUGINS_ATTRIBUTES_TO_DUMP})
+ plugins_info.append(info)
- for attr_name in PLUGINS_MANAGER_ATTRIBUTES_TO_DUMP:
- attr_value: Optional[List[Any]] = getattr(plugins_manager, attr_name)
- if not attr_value:
- continue
- table = SimpleTable(title=attr_name.capitalize().replace("_", " "))
- table.add_column(width=100)
- for item in attr_value: # pylint: disable=not-an-iterable
- table.add_row(
- f"- {_get_name(item)}",
- )
- console.print(table)
+ # Remove empty info
+ if args.output == "table": # pylint: disable=too-many-nested-blocks
+ for col in list(plugins_info[0].keys()): # it will exist as there's at least one plugin
Review comment:
Extended the comment. It was mostly about the `xs[0]` as normally the element my not exists, but in this case we are sure there will be at least on plugin - we check it few lines earlier.
----------------------------------------------------------------
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] XD-DENG merged pull request #13036: Refactor plugins command output using AirflowConsole
Posted by GitBox <gi...@apache.org>.
XD-DENG merged pull request #13036:
URL: https://github.com/apache/airflow/pull/13036
----------------------------------------------------------------
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] XD-DENG commented on a change in pull request #13036: Refactor plugins command output using AirflowConsole
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on a change in pull request #13036:
URL: https://github.com/apache/airflow/pull/13036#discussion_r541785722
##########
File path: airflow/cli/simple_table.py
##########
@@ -53,13 +56,18 @@ def print_as_table(self, data: List[Dict]):
table.add_row(*[str(d) for d in row.values()])
self.print(table)
- def _normalize_data(self, value: Any, output: str) -> Union[list, str, dict]:
+ # pylint: disable=too-many-return-statements
+ def _normalize_data(self, value: Any, output: str) -> Optional[Union[list, str, dict]]:
if isinstance(value, (tuple, list)):
if output == "table":
return ",".join(self._normalize_data(x, output) for x in value)
return [self._normalize_data(x, output) for x in value]
if isinstance(value, dict) and output != "table":
return {k: self._normalize_data(v, output) for k, v in value.items()}
+ if isinstance(value, PluginsDirectorySource):
+ return str(value)
+ if inspect.isclass(value):
+ return value.__name__
Review comment:
I understand that we have these two `IF`s in order here because `PluginsDirectorySource ` is a special `class` to handle (we want to use its `__str__` rather than `__name__`). But the first glance at it is a bit confusing because what it returns _looks the same_ to the last return line. This may be a confusion to people who read this later.
So I would like to suggest changing it to
```python
if inspect.isclass(value) and not isinstance(value, PluginsDirectorySource):
return value.__name__
```
##########
File path: airflow/cli/commands/plugins_command.py
##########
@@ -78,33 +75,17 @@ def dump_plugins(args):
print("No plugins loaded")
return
- console = Console()
- console.print("[bold yellow]SUMMARY:[/bold yellow]")
- console.print(
- f"[bold green]Plugins directory[/bold green]: {conf.get('core', 'plugins_folder')}\n", highlight=False
- )
- console.print(
- f"[bold green]Loaded plugins[/bold green]: {len(plugins_manager.plugins)}\n", highlight=False
- )
+ plugins_info: List[Dict[str, str]] = []
+ for plugin in plugins_manager.plugins:
+ info = {"name": plugin.name}
+ info.update({n: getattr(plugin, n) for n in PLUGINS_ATTRIBUTES_TO_DUMP})
+ plugins_info.append(info)
- for attr_name in PLUGINS_MANAGER_ATTRIBUTES_TO_DUMP:
- attr_value: Optional[List[Any]] = getattr(plugins_manager, attr_name)
- if not attr_value:
- continue
- table = SimpleTable(title=attr_name.capitalize().replace("_", " "))
- table.add_column(width=100)
- for item in attr_value: # pylint: disable=not-an-iterable
- table.add_row(
- f"- {_get_name(item)}",
- )
- console.print(table)
+ # Remove empty info
+ if args.output == "table": # pylint: disable=too-many-nested-blocks
+ for col in list(plugins_info[0].keys()): # it will exist as there's at least one plugin
Review comment:
```suggestion
for col in plugins_info[0]: # it will exist as there's at least one plugin
```
----------------------------------------------------------------
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] XD-DENG commented on a change in pull request #13036: Refactor plugins command output using AirflowConsole
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on a change in pull request #13036:
URL: https://github.com/apache/airflow/pull/13036#discussion_r541882382
##########
File path: airflow/cli/commands/plugins_command.py
##########
@@ -78,33 +75,17 @@ def dump_plugins(args):
print("No plugins loaded")
return
- console = Console()
- console.print("[bold yellow]SUMMARY:[/bold yellow]")
- console.print(
- f"[bold green]Plugins directory[/bold green]: {conf.get('core', 'plugins_folder')}\n", highlight=False
- )
- console.print(
- f"[bold green]Loaded plugins[/bold green]: {len(plugins_manager.plugins)}\n", highlight=False
- )
+ plugins_info: List[Dict[str, str]] = []
+ for plugin in plugins_manager.plugins:
+ info = {"name": plugin.name}
+ info.update({n: getattr(plugin, n) for n in PLUGINS_ATTRIBUTES_TO_DUMP})
+ plugins_info.append(info)
- for attr_name in PLUGINS_MANAGER_ATTRIBUTES_TO_DUMP:
- attr_value: Optional[List[Any]] = getattr(plugins_manager, attr_name)
- if not attr_value:
- continue
- table = SimpleTable(title=attr_name.capitalize().replace("_", " "))
- table.add_column(width=100)
- for item in attr_value: # pylint: disable=not-an-iterable
- table.add_row(
- f"- {_get_name(item)}",
- )
- console.print(table)
+ # Remove empty info
+ if args.output == "table": # pylint: disable=too-many-nested-blocks
+ for col in list(plugins_info[0].keys()): # it will exist as there's at least one plugin
Review comment:
Ja, correct. My bad.
How about the comment? May you clarify?
----------------------------------------------------------------
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] XD-DENG commented on a change in pull request #13036: Refactor plugins command output using AirflowConsole
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on a change in pull request #13036:
URL: https://github.com/apache/airflow/pull/13036#discussion_r541787038
##########
File path: airflow/cli/commands/plugins_command.py
##########
@@ -78,33 +75,17 @@ def dump_plugins(args):
print("No plugins loaded")
return
- console = Console()
- console.print("[bold yellow]SUMMARY:[/bold yellow]")
- console.print(
- f"[bold green]Plugins directory[/bold green]: {conf.get('core', 'plugins_folder')}\n", highlight=False
- )
- console.print(
- f"[bold green]Loaded plugins[/bold green]: {len(plugins_manager.plugins)}\n", highlight=False
- )
+ plugins_info: List[Dict[str, str]] = []
+ for plugin in plugins_manager.plugins:
+ info = {"name": plugin.name}
+ info.update({n: getattr(plugin, n) for n in PLUGINS_ATTRIBUTES_TO_DUMP})
+ plugins_info.append(info)
- for attr_name in PLUGINS_MANAGER_ATTRIBUTES_TO_DUMP:
- attr_value: Optional[List[Any]] = getattr(plugins_manager, attr_name)
- if not attr_value:
- continue
- table = SimpleTable(title=attr_name.capitalize().replace("_", " "))
- table.add_column(width=100)
- for item in attr_value: # pylint: disable=not-an-iterable
- table.add_row(
- f"- {_get_name(item)}",
- )
- console.print(table)
+ # Remove empty info
+ if args.output == "table": # pylint: disable=too-many-nested-blocks
+ for col in list(plugins_info[0].keys()): # it will exist as there's at least one plugin
Review comment:
And I'm not sure if the comment is correct. I assume it's possible to have zero plugin?
----------------------------------------------------------------
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] XD-DENG commented on a change in pull request #13036: Refactor plugins command output using AirflowConsole
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on a change in pull request #13036:
URL: https://github.com/apache/airflow/pull/13036#discussion_r541783293
##########
File path: airflow/cli/commands/plugins_command.py
##########
@@ -15,17 +15,14 @@
# specific language governing permissions and limitations
# under the License.
import inspect
-from typing import Any, List, Optional, Union
-
-from rich.console import Console
+from typing import Any, Dict, List, Union
from airflow import plugins_manager
-from airflow.cli.simple_table import SimpleTable
-from airflow.configuration import conf
+from airflow.cli.simple_table import AirflowConsole
from airflow.plugins_manager import PluginsDirectorySource
+from airflow.utils.cli import suppress_logs_and_warning
# list to maintain the order of items.
-from airflow.utils.cli import suppress_logs_and_warning
PLUGINS_MANAGER_ATTRIBUTES_TO_DUMP = [
Review comment:
`PLUGINS_MANAGER_ATTRIBUTES_TO_DUMP` can be removed now 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #13036: Refactor plugins command output using AirflowConsole
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #13036:
URL: https://github.com/apache/airflow/pull/13036#discussion_r541824087
##########
File path: airflow/cli/commands/plugins_command.py
##########
@@ -78,33 +75,17 @@ def dump_plugins(args):
print("No plugins loaded")
return
- console = Console()
- console.print("[bold yellow]SUMMARY:[/bold yellow]")
- console.print(
- f"[bold green]Plugins directory[/bold green]: {conf.get('core', 'plugins_folder')}\n", highlight=False
- )
- console.print(
- f"[bold green]Loaded plugins[/bold green]: {len(plugins_manager.plugins)}\n", highlight=False
- )
+ plugins_info: List[Dict[str, str]] = []
+ for plugin in plugins_manager.plugins:
+ info = {"name": plugin.name}
+ info.update({n: getattr(plugin, n) for n in PLUGINS_ATTRIBUTES_TO_DUMP})
+ plugins_info.append(info)
- for attr_name in PLUGINS_MANAGER_ATTRIBUTES_TO_DUMP:
- attr_value: Optional[List[Any]] = getattr(plugins_manager, attr_name)
- if not attr_value:
- continue
- table = SimpleTable(title=attr_name.capitalize().replace("_", " "))
- table.add_column(width=100)
- for item in attr_value: # pylint: disable=not-an-iterable
- table.add_row(
- f"- {_get_name(item)}",
- )
- console.print(table)
+ # Remove empty info
+ if args.output == "table": # pylint: disable=too-many-nested-blocks
+ for col in list(plugins_info[0].keys()): # it will exist as there's at least one plugin
Review comment:
The list should stay there. It's to make sure we iterate over fixed range - the dict may change over time due to `del` 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