betodealmeida commented on code in PR #30833:
URL: https://github.com/apache/superset/pull/30833#discussion_r1851121212
##########
superset/commands/chart/importers/v1/__init__.py:
##########
@@ -46,7 +49,14 @@ class ImportChartsCommand(ImportModelsCommand):
import_error = ChartImportError
@staticmethod
- def _import(configs: dict[str, Any], overwrite: bool = False) -> None:
+ def _import(
+ configs: dict[str, Any],
+ overwrite: bool = False,
+ contents: Optional[dict[str, Any]] = None,
Review Comment:
Nit, no need for `Optional`:
```suggestion
contents: dict[str, Any] | None = None,
```
(You might need to include `from __future__ import annotations`.)
##########
superset/commands/chart/export.py:
##########
@@ -71,9 +74,23 @@ def _file_content(model: Slice) -> str:
if model.table:
payload["dataset_uuid"] = str(model.table.uuid)
+ # Fetch tags from the database if TAGGING_SYSTEM is enabled
+ if feature_flag_manager.is_feature_enabled("TAGGING_SYSTEM"):
+ tags = model.tags if hasattr(model, "tags") else []
Review Comment:
Small nit:
```suggestion
tags = getattr(model, "tags", [])
```
##########
superset/commands/dashboard/importers/v1/__init__.py:
##########
@@ -15,7 +15,7 @@
# specific language governing permissions and limitations
# under the License.
-from typing import Any
+from typing import Any, Optional
Review Comment:
Same here.
##########
superset/commands/export/assets.py:
##########
@@ -50,15 +51,17 @@ def run(self) -> Iterator[tuple[str, Callable[[], str]]]:
ExportDatabasesCommand,
ExportDatasetsCommand,
ExportChartsCommand,
+ ExportTagsCommand,
ExportDashboardsCommand,
ExportSavedQueriesCommand,
]
for command in commands:
- ids = [model.id for model in command.dao.find_all()]
- for file_name, file_content in command(ids,
export_related=False).run():
- if file_name not in seen:
- yield file_name, file_content
- seen.add(file_name)
+ if hasattr(command, "dao"):
Review Comment:
I'm confused by this — does `ExportTagsCommand` have a DAO? If it has, why
the `hasattr` check; if it doesn't, why include it in `commands`?
##########
superset/commands/chart/export.py:
##########
@@ -85,3 +102,12 @@ def _export(
if model.table and export_related:
yield from ExportDatasetsCommand([model.table.id]).run()
+
+ # Check if the calling class is ExportDashboardCommands
+ if (
+ export_related
+ and ExportChartsCommand._include_tags
+ and feature_flag_manager.is_feature_enabled("TAGGING_SYSTEM")
+ ):
+ chart_id = model.id
+ yield from ExportTagsCommand().export(chart_ids=[chart_id])
Review Comment:
Why only include tags for dashboard exports? Why not always include tags
here?
##########
superset/commands/chart/importers/v1/__init__.py:
##########
@@ -15,21 +15,24 @@
# specific language governing permissions and limitations
# under the License.
-from typing import Any
+from typing import Any, Optional
Review Comment:
```suggestion
from typing import Any
```
##########
superset/commands/importers/v1/utils.py:
##########
@@ -214,3 +218,79 @@ def get_contents_from_bundle(bundle: ZipFile) -> dict[str,
str]:
for file_name in bundle.namelist()
if is_valid_config(file_name)
}
+
+
+# pylint: disable=consider-using-transaction
+def import_tag(
+ new_tag_names: list[str],
+ contents: dict[str, Any],
+ object_id: int,
+ object_type: str,
+ db_session: Session,
+) -> list[int]:
+ """Handles the import logic for tags for charts and dashboards"""
+
+ if not feature_flag_manager.is_feature_enabled("TAGGING_SYSTEM"):
+ return []
+
+ tag_descriptions = {}
+ new_tag_ids = []
+ if "tags.yaml" in contents:
+ try:
+ tags_config = yaml.safe_load(contents["tags.yaml"])
+ for tag_info in tags_config.get("tags", []):
+ tag_name = tag_info.get("tag_name")
+ description = tag_info.get("description", None)
+ if tag_name:
+ tag_descriptions[tag_name] = description
+ except yaml.YAMLError as err: # Renamed to 'err' for clarity
+ logger.error("Error parsing tags.yaml: %s", err) # Used lazy
logging
Review Comment:
It's usually better to have small try/except blocks:
```suggestion
try:
tags_config = yaml.safe_load(contents["tags.yaml"])
except yaml.YAMLError as err: # Renamed to 'err' for clarity
logger.error("Error parsing tags.yaml: %s", err) # Used lazy
logging
tags_config = {}
for tag_info in tags_config.get("tags", []):
tag_name = tag_info.get("tag_name")
description = tag_info.get("description", None)
if tag_name:
tag_descriptions[tag_name] = description
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
