-
Notifications
You must be signed in to change notification settings - Fork 14.5k
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: Add dataset tagging to the back-end #20892
Conversation
Codecov Report
@@ Coverage Diff @@
## master #20892 +/- ##
==========================================
+ Coverage 66.66% 66.68% +0.01%
==========================================
Files 1793 1794 +1
Lines 68492 68562 +70
Branches 7277 7277
==========================================
+ Hits 45663 45722 +59
- Misses 20967 20978 +11
Partials 1862 1862
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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 you add some unit tests to make sure that the commands are working as expected.
Closing this PR so that I can add unit tests to it. I will create a new PR once that is done. |
…ted object was deleted
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.
LGTM! 👍 One minor comment regarding the tagging system fixture, but I don't consider it blocking, as flipping a default feature flag will always require some test refactoring anyway.
Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>
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.
Looks good!
One question, why the move of /superset/models/tags.py
to /superset/tags/models.py
?
Note: We are currently moving and deprecating all endpoints from /supeset
to /api/v1
superset/migrations/versions/2018-07-26_11-10_c82ee8a39623_add_implicit_tags.py
Show resolved
Hide resolved
The requested changes have been made
FYI @AAfghahi I removed the change request from your review as the requested tests have been added to the PR |
SUMMARY
In the current version of the master branch, the back-end only supports tagging for 'Charts', 'Dashboards' and 'Saved Queries'. However, this PR would add the back-end support for tagging 'Datasets' as well. This is just the first of at least 2 PR's, the second of which will introduce new API endpoints as well as add dataset tags to the front-end.
This PR also incorporates a few changes to files for refactoring purposes:
superset/models/tags.py
tosuperset/tags/models.py
(all of the import statements have been updated to point to the new location). This was done so that going forward, the model will be in the same location as the API and command code, which will be added later on.superset/tags/core.py
. The event listeners are now registered from within theinit_app_in_ctx
method in thesuperset/initialization/__init__.py
file, and are placed behind a check for theTAGGING_SYSTEM
feature flag.NOTE: If you are not starting from a 'fresh' database, be sure to run the following command:
superset sync_tags
. This will add tags to all of the existing datasets.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
TESTING INSTRUCTIONS
To manually verify the changes, first connect to the local database, then run the following:
Before Code Changes:
SELECT * FROM tag;
(you should not see a dataset entry)SELECT * FROM tagged_object;
(you should not see any entry with the dataset tag/type)After Code Changes:
SELECT * FROM tag;
(you should now see a dataset entry)SELECT * FROM tagged_object;
(you should now see some entries with the dataset tag/type)ADDITIONAL INFORMATION