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.

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

fix(ingest): ingest deleted looker dashboards #3158

Merged
merged 1 commit into from
Sep 8, 2021

Conversation

karoliskascenas
Copy link
Contributor

@karoliskascenas karoliskascenas commented Aug 25, 2021

all_dashboards() does not return deleted dashboards (even says so in the comments).

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)

@frsann
Copy link
Contributor

frsann commented Aug 30, 2021

Can you make that addition optional at least? We have no use of deleted dashboards in our datahub. Or are you more interested in marking previously ingested dashboards as deleted, so that they disappear in datahub?

@karoliskascenas
Copy link
Contributor Author

karoliskascenas commented Aug 30, 2021

Or are you more interested in marking previously ingested dashboards as deleted, so that they disappear in datahub?

That's exactly what I am trying to achieve.

@frsann
Copy link
Contributor

frsann commented Aug 30, 2021

But as a side effect you will be "ingesting" all deleted ones as well. They wont show up in the UI, but they will be stored in the database. Maybe not a huge problem, but worth thinking about.

@karoliskascenas
Copy link
Contributor Author

karoliskascenas commented Aug 30, 2021

Judging by the code that adds a status aspect this was always the intended functionality - it just never had a chance to work.

@frsann
Copy link
Contributor

frsann commented Aug 30, 2021

Sure, but it might be good to add a note about this in the docs ("be very conservative with the allow-pattern"). We are at least closing in on 5 digit dashboard IDs (I assume they are strictly increasing) as users create and delete dashboards as the see fit, and I certainly don't want to re-ingest it on a, say, daily basis.

shirshanka
shirshanka previously approved these changes Aug 31, 2021
Copy link
Contributor

@shirshanka shirshanka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@shirshanka
Copy link
Contributor

@frsann makes a good point, it may not be a great idea to query looker for all dashboards (deleted + not-deleted) also.

@shirshanka shirshanka dismissed their stale review August 31, 2021 06:59

Didn't review this too carefully, we'll have to think about a different way to accomplish deletes IMO

@karoliskascenas
Copy link
Contributor Author

Would making the ingestion of deleted dashboards optional work for you?

Copy link
Contributor

@shirshanka shirshanka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@shirshanka shirshanka merged commit 792a08c into datahub-project:master Sep 8, 2021
@karoliskascenas karoliskascenas deleted the patch-1 branch September 8, 2021 05:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants