Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(ingest): allow for deleting aspects in cli with -p tag #6190

Merged
merged 4 commits into from
Oct 25, 2022
Merged

feat(ingest): allow for deleting aspects in cli with -p tag #6190

merged 4 commits into from
Oct 25, 2022

Conversation

Anirudh181001
Copy link
Contributor

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
  • For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

@github-actions github-actions bot added the ingestion PR or Issue related to the ingestion of metadata label Oct 13, 2022
@Anirudh181001
Copy link
Contributor Author

In the datahub docs, the following example is included:

# Delete all of the aspect values for a given platform and a timeseries aspect.
datahub delete -p "<platform>" -a "<timeseries aspect>" --hard
Eg: datahub delete -p "snowflake" -a "datasetProfile" --hard

However, there is a small bug in which the aspect information is not passed on when a delete command is run with the -p flag. Moreover, this command also cannot be run without the --hard flag since there is no soft delete for an aspect.
This PR addresses the above concerns and provide a simple fix for it.

@maggiehays maggiehays added the community-contribution PR or Issue raised by member(s) of DataHub Community label Oct 13, 2022
@hsheth2
Copy link
Collaborator

hsheth2 commented Oct 14, 2022 •
edited
Loading

@Anirudh181001 - thanks for the PR!

It overall looks good. Given that we don't have unit tests for delete, I just wanted to check if you tested this locally?

@github-actions
Copy link

github-actions bot commented Oct 14, 2022 •
edited
Loading

Unit Test Results (metadata ingestion)

       8 files  ±0         8 suites  ±0   58m 10s ⏱️ + 2m 4s
   748 tests ±0     745 ✔️ ±0  3 💤 ±0  0 ❌ ±0 
1 498 runs  ±0  1 492 ✔️ ±0  6 💤 ±0  0 ❌ ±0 

Results for commit 55a7c3d. ± Comparison against base commit 26b4a9e.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Oct 14, 2022 •
edited
Loading

Unit Test Results (build & test)

597 tests  ±0   593 ✔️ ±0   11m 49s ⏱️ +5s
147 suites ±0       4 💤 ±0 
147 files   ±0       0 ❌ ±0 

Results for commit 55a7c3d. ± Comparison against base commit 26b4a9e.

♻️ This comment has been updated with latest results.

@Anirudh181001
Copy link
Contributor Author

Anirudh181001 commented Oct 17, 2022 •
edited
Loading

@Anirudh181001 - thanks for the PR!

It overall looks good. Given that we don't have unit tests for delete, I just wanted to check if you tested this locally?

Hi @hsheth2 I tested this locally! Seems to work fine! :)

@hsheth2
Copy link
Collaborator

hsheth2 commented Oct 18, 2022

@Anirudh181001 awesome!

Looks like there's a tiny lint error - could you just run ./gradlew :metadata-ingestion:lintFix?

@Anirudh181001
Copy link
Contributor Author

Anirudh181001 commented Oct 21, 2022 •
edited
Loading

@Anirudh181001 awesome!

Looks like there's a tiny lint error - could you just run ./gradlew :metadata-ingestion:lintFix?

Yup fixed now! Can we go ahead and merge?

@maggiehays maggiehays added the hacktoberfest-accepted Acceptance for hacktoberfest https://hacktoberfest.com/participation/ label Oct 21, 2022
@hsheth2 hsheth2 changed the title Allow for deleting aspects in cli with -p tag feat(ingest): allow for deleting aspects in cli with -p tag Oct 25, 2022
@hsheth2 hsheth2 merged commit aeed817 into datahub-project:master Oct 25, 2022
cccs-tom pushed a commit to CybercentreCanada/datahub that referenced this pull request Nov 18, 2022
@Anirudh181001 @hsheth2 @cccs-tom
feat(ingest): allow for deleting aspects in cli with -p tag (datahub-…
86d9541 
…project#6190)

Co-authored-by: Harshal Sheth <[email protected]>
cccs-tom pushed a commit to CybercentreCanada/datahub that referenced this pull request Nov 18, 2022
@Anirudh181001 @hsheth2 @cccs-tom
feat(ingest): allow for deleting aspects in cli with -p tag (datahub-…
5b1f2a2 
…project#6190)

Co-authored-by: Harshal Sheth <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hsheth2 hsheth2 hsheth2 approved these changes

Assignees
No one assigned
Labels
community-contribution PR or Issue raised by member(s) of DataHub Community hacktoberfest-accepted Acceptance for hacktoberfest https://hacktoberfest.com/participation/ ingestion PR or Issue related to the ingestion of metadata
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Anirudh181001 @hsheth2 @maggiehays