-
Notifications
You must be signed in to change notification settings - Fork 886
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
Fixes #16263 : Mode Ingestion not ingesting any records bug fix #16264
base: main
Are you sure you want to change the base?
Conversation
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
@@ -75,7 +75,7 @@ def fetch_all_reports(self, workspace_name: str) -> Optional[list]: | |||
dict | |||
""" | |||
all_reports = [] | |||
response_collections = self.client.get(f"/{workspace_name}/{COLLECTIONS}") | |||
response_collections = self.client.get(f"/{workspace_name}/{COLLECTIONS}?filter=all") |
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.
filter=all param is added to get records from collections API
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.
@dixon2678 this looks good to me does this need any special permissions?
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.
@ulixius9 I have added my reply on the latest change 🙇♂️
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
@@ -75,7 +75,7 @@ def fetch_all_reports(self, workspace_name: str) -> Optional[list]: | |||
dict | |||
""" | |||
all_reports = [] | |||
response_collections = self.client.get(f"/{workspace_name}/{COLLECTIONS}?filter=all") | |||
response_collections = self.client.get(f"/{workspace_name}/{COLLECTIONS}?filter=custom") |
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.
Hi @ulixius9 I have made this change from all
to custom
To answer your inquiry, all API Workspace tokens in Mode will have admin privileges by default (also only admins are able to create them) and according to the docs, it looks like granular permission control are not yet supported.
Personal tokens that support granular permssion control to Mode resources appears to be deprecated according to this docs, and no new personal tokens are able to be created as of now. This will be fully deprecated by August.
Several points when using the new Workspace Token:
- At least in my case here, not specifying the filter parameter returns no collections. I have tried it with both Restricted and Unrestricted (default access) collections.
- Specifying
custom
as a filter returns all collections in the Workspace, except Personal collections -> I think this is the most appropriate approach. - Specifying 'all' returns all collections including Personal collections.
However, there is no way that I could confirm whether the existing users that are still using the deprecated Personal tokens (with limited access, as opposed to the new Workspace token method that grants default admin access) are able to use the filter=custom
params without any errors, as I am also unable to create a new Personal token in Mode.
Perhaps should we hold off this PR until the usage of exisitng Personal Tokens are fully deprecated in August 2024? @ulixius9
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.
Thanks @dixon2678 for this details explanation, IMO we can approach it this way -> we introduce a new field in the service connection form so that user can configure this filter i.e wether they want an all
, custom
or no filter at all
this way it is possible for users to ingest or skip personal collections and also possible for them to use the deprecated version if they still decide to use is, what do you think about this approach?
and we can keep no filter as a default selection so it does not affect the existing users
@@ -67,15 +67,22 @@ def __init__(self, config): | |||
) | |||
self.client = REST(client_config) | |||
|
|||
def fetch_all_reports(self, workspace_name: str) -> Optional[list]: | |||
def fetch_all_reports(self, workspace_name: str, filter: Optional[str] = None) -> Optional[list]: |
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 have added filter as an argument with the default value of None (i.e. no filter). What do you think? @ulixius9
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
Quality Gate passed for 'open-metadata-ingestion'Issues Measures |
Describe your changes:
Fixes #16263
I worked on changing the collections API get collection method by adding a filter=custom param because no records will be returned if this param is not included.
Reference Mode API docs
Type of change:
Checklist:
Fixes <issue-number>: <short explanation>