-
Notifications
You must be signed in to change notification settings - Fork 323
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
add DELETE end point for dataset tags #2698
add DELETE end point for dataset tags #2698
Conversation
Signed-off-by: sharpd <number6labs@gmail.com>
✅ Deploy Preview for peppy-sprite-186812 canceled.
|
Signed-off-by: sharpd <number6labs@gmail.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2698 +/- ##
============================================
+ Coverage 84.24% 84.27% +0.03%
- Complexity 1405 1407 +2
============================================
Files 249 249
Lines 6371 6386 +15
Branches 291 291
============================================
+ Hits 5367 5382 +15
Misses 851 851
Partials 153 153 ☔ View full report in Codecov by Sentry. |
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.
I really like @wslulciuc approach to inregration tests introduced here -> #2700
I know the testing approach is newer than this PR, but the code here seems to be the perfect candidate to write a neat integration test. This will also require implenting tag deleting feature in Marquez client.
SELECT 1 | ||
FROM datasets d | ||
JOIN tags t ON d.uuid = dtm.dataset_uuid AND t.uuid = dtm.tag_uuid | ||
WHERE d.name = :datasetName AND t.name = :tagName |
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.
Shouldn't where condition contain namespace filter as well?
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.
I am guessing you are thinking of the scenario where a dataset name could span 2 namespaces?
Cool will add.
Signed-off-by: sharpd <number6labs@gmail.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.
LGTM! 💯 🥇
Problem
Currently we have no way via the API to a delete a tag from a dataset meaning datasets could potentially be "mis tagged"
Closes: #2692
Solution
Solution is to create a DELETE end point which removes the linkage between the dataset and the tag in
One-line summary:
Add DELETE end point for dataset tags.
CHANGELOG.md
(Depending on the change, this may not be necessary)..sql
database schema migration according to Flyway's naming convention (if relevant)