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

feat(ingest/metabase): add ability to exclude other users collections #10330

Merged

Conversation

paguos
Copy link
Contributor

@paguos paguos commented Apr 18, 2024

Closes #10328

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 ingestion PR or Issue related to the ingestion of metadata community-contribution PR or Issue raised by member(s) of DataHub Community labels Apr 18, 2024
@paguos
Copy link
Contributor Author

paguos commented Apr 18, 2024

Related to issue #10328

Copy link
Collaborator

@hsheth2 hsheth2 left a comment

Choose a reason for hiding this comment

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

One nit, otherwise looks good

@@ -79,6 +79,10 @@ class MetabaseConfig(DatasetLineageProviderConfigBase):
default="public",
description="Default schema name to use when schema is not provided in an SQL query",
)
exclude_other_user_collections: str = Field(
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be a bool type here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense

@@ -209,6 +213,7 @@ def emit_dashboard_mces(self) -> Iterable[MetadataWorkUnit]:
try:
collections_response = self.session.get(
f"{self.config.connect_uri}/api/collection/"
f"?exclude-other-user-collections={self.config.exclude_other_user_collections}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

and then we can json.dumps the bool value to convert it to false or true

@paguos paguos force-pushed the feat/metabase-excluse-other-users branch 2 times, most recently from ec01c6c to f72cb62 Compare April 19, 2024 08:13
Signed-off-by: Pablo Osinaga <pablo.osinaga@deepl.com>
@paguos paguos force-pushed the feat/metabase-excluse-other-users branch from f72cb62 to cc3eb5a Compare April 19, 2024 08:16
@paguos
Copy link
Contributor Author

paguos commented Apr 19, 2024

@hsheth2 thank you for the review!

  • I made the changes you requested
  • Fixed the integration test

I amend my commit, so its difficult to see the changes (sorry).

@hsheth2 hsheth2 added the merge-pending-ci A PR that has passed review and should be merged once CI is green. label Apr 19, 2024
@hsheth2 hsheth2 changed the title feat(metabase): add ability to exclude other users collections feat(ingest/metabase): add ability to exclude other users collections Apr 19, 2024
@hsheth2 hsheth2 merged commit 62c7ac7 into datahub-project:master Apr 19, 2024
57 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-contribution PR or Issue raised by member(s) of DataHub Community ingestion PR or Issue related to the ingestion of metadata merge-pending-ci A PR that has passed review and should be merged once CI is green.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(Metabase): ingestion lacks ability to omit personal collections
2 participants