-
Notifications
You must be signed in to change notification settings - Fork 13.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(tags): Export and Import Functionality for Superset Dashboards and Charts #30833
base: master
Are you sure you want to change the base?
Conversation
DR-4503: Added export Tags functionality for Superset Dashboards and …
DR-4503: Added Import Tags functionality for Superset Dashboards and Charts
DR-4503: Fix Export Tag to ignore owner:
remove docker .env custom SUPERSET_SECRET_KEY
Superset remove unwanted files
Thank you for the PR @asher-lab. Could you add unit tests for the new feature with the feature flag on and off? |
Add superset logic to handle disable tagging system
Good day, @michael-s-molina. We've added logic to handle cases when the Tagging System is disabled in Superset. When disabled, the Could you provide guidance on creating a unit test for this new feature? I have limited experience with writing unit tests, so any help would be appreciated. Thank you! |
Hi @asher-lab. You can check this file for examples. |
Add Unit Testing for Tag Export and Import in Superset
Hi @michael-s-molina . Added unit tests for the new feature with the feature flag TAGGING_SYSTEM on and off. Thank you. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #30833 +/- ##
==========================================
+ Coverage 60.48% 65.35% +4.86%
==========================================
Files 1931 537 -1394
Lines 76236 39053 -37183
Branches 8568 0 -8568
==========================================
- Hits 46114 25522 -20592
+ Misses 28017 13531 -14486
+ Partials 2105 0 -2105
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
I seen a bunch of tests: 10 are failing and 23 are successful. Could you provide what tests that I need to look at? |
superset/charts/schemas.py
Outdated
@@ -1550,7 +1550,7 @@ class ImportV1ChartSchema(Schema): | |||
dataset_uuid = fields.UUID(required=True) | |||
is_managed_externally = fields.Boolean(allow_none=True, dump_default=False) | |||
external_url = fields.String(allow_none=True) | |||
|
|||
tag = fields.List(fields.String(), allow_none=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we have it named as tags
, since it's a list?
superset/commands/chart/export.py
Outdated
model.tags if hasattr(model, "tags") else [] | ||
) | ||
# Filter out any tags that contain "type:" in their name | ||
payload["tag"] = [tag.name for tag in tags if not any(x in tag.name for x in ["type:", "owner:"])] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since users can create tags with those strings, could we filter for type, instead? something like:
payload["tags"] = [tag.name for tag in tags if tag.type == TagType.custom]
superset/dashboards/schemas.py
Outdated
@@ -453,7 +453,7 @@ class ImportV1DashboardSchema(Schema): | |||
certified_by = fields.String(allow_none=True) | |||
certification_details = fields.String(allow_none=True) | |||
published = fields.Boolean(allow_none=True) | |||
|
|||
tag = fields.List(fields.String(), allow_none=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
tags = ( | ||
model.tags if hasattr(model, "tags") else [] | ||
) | ||
payload["tag"] = [tag.name for tag in tags if not any(prefix in tag.name for prefix in ["type:", "owner:"])] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
Thank you so much @asher-lab! This is super cool and very useful 😍 🙌 I left a few comments, and I plan to give it another pass Tomorrow. |
Fix integration test superset, change to "tags" and update tag filter using TagType apache#30869
Hi @Vitor-Avila , we have fixed the integration test issue, updated it to use "tags" instead of "tag" and update filter using TagType. Thank you! |
Hi @Vitor-Avila and @michael-s-molina, if these are merged in, when is the expected date it be released? |
Running CI 🤞 This would definitely be part of 5.0, which needs a new estimated date. It will have missed the boat for 4.1.0, but could make a 4.2.0, though it's unclear if/when that would happen. It would not make a 4.1.1 release since that would only include fixes. Feel free to join us on slack in the release strategy or release announcement channels, if you want to get involved with the process. |
Currently fixing the tests that are failing. Will be making a commit in order to fix those. Thank you. I realized that we need to work with pre-commit checks and integration tests... |
* Added self.contents in database and datasets for import * Add self.content in query for import * Run precommit and fix some issues in superset * Add ExportTagsCommand * Fix ImportExamplesCommand arguments to make Optional * Remove should_export_tags parameter to solve pre-commit error * Fix ExportDashboard command calling ExportTagsCommandfrom ExportChartsCommand * Remove protected access for ExportTagsCommand _export * Fix some Pylint issues * Ensure Feature Flag for TAGGING_SYSTEM is turned off --------- Co-authored-by: Asher Manangan <[email protected]>
Good morning team, @rusackas @Vitor-Avila and @michael-s-molina. Could I get another run for this one please. I did run it on my own fork and all tests have passed. 🤞 |
hey @asher-lab, |
SUMMARY
This PR is related to Discussion: #30629 by Spens
Motivation
We have adopted the tagging functionality and it is working quite well for us for dashboards and and charts. Our use case and process requires us to build dashboards on a development server, export the dashboards, commit the YAMLs to source control, and finally import the ZIPs to our production server. We would like the tags we assign on the development server to be captured in the export so they are replicated to our production server.
Proposed Change
UI elements are not affected by this change. All changes are related to updated logic/processing of the existing export and import functionality.
Only tags of type "custom" are exported. Other tags that exist but are not exported are "type" and "owner".
Export Logic
When an object (Dashboard/Chart) is exported, a new parameter is added to the exported YAML file. The parameter is "tags" and the value is an array of tag names. Here is an example:
...
Because tags have additional information (for example, a description), a new file is needed to hold this information. The file is called "tags.yaml" and lives in the top level of the zip file. For example:
The format of the tags.yaml file is an array of tags
Import Logic
When an object is imported, the code checks to see if any tags included in the import file.
If yes, the tag is created if it does not already exist then the tag is assigned to the imported object. If the tag already exists, it is just assigned with no changes to the tag.
Details
Existing Superset behavior: When importing objects that already exist, the user is prompted whether it is ok to overwrite. If no, the import process aborts. If yes, the old object is replaced with the new object.
To be consistent with this behavior, when importing an object and specifying "overwrite", old tags that are no longer specified with the object are removed, new tags that didn't originally exist are created, old tags that still remain in the new set of tags are retained (descriptions are not updated for example)
This is intentional because for the use case of trying to mirror the settings and properties from one system to another, you must have the ability to remove tags from the target system. Otherwise, for example, if you rename a tag, the new name would get added and the old name would stick around which is not the desired behavior.
Thank you to anyone taking the time to read and comment!
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION