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(ingestion/looker): set project-name for imported_projects views #8086

Conversation

siddiquebagwan
Copy link
Contributor

No description provided.

@github-actions github-actions bot added the ingestion PR or Issue related to the ingestion of metadata label May 19, 2023
@siddiquebagwan siddiquebagwan changed the title fix(ingestion/looker): configurable project-name for looker explore's view fix(ingestion/looker): set project-name for imported_projects views May 22, 2023
view_project_map: Dict[str, str] = {}
for view_field in view_fields:
if view_field.view_name is not None and view_field.project_name is not None:
view_project_map[view_field.view_name] = view_field.project_name
Copy link
Collaborator

Choose a reason for hiding this comment

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

does this break if you have the same view name in two projects?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

view within a model has a unique name, so it wouldn't break as it is getting used by LookerExplore.from_api.
I added the respective doc string in the function.

…:acryldata/datahub-fork into master+ing-74-looker-refinements-lineage
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 minor comment but otherwise LGTM

Just to make sure, the new looker_config.py file is purely a refactoring, and there's no code changes in there right?

@@ -734,7 +691,9 @@ def from_api( # noqa: C901
fields=view_fields,
upstream_views=list(
ProjectInclude(
project=_BASE_PROJECT_NAME,
project=_BASE_PROJECT_NAME
if view_name not in view_project_map
Copy link
Collaborator

Choose a reason for hiding this comment

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

can change this to view_project_map.get(view_name, _BASE_PROJECT_NAME)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@siddiquebagwan
Copy link
Contributor Author

one minor comment but otherwise LGTM

Just to make sure, the new looker_config.py file is purely a refactoring, and there's no code changes in there right?

Yup, It is just refactoring.

@hsheth2 hsheth2 merged commit e7d1b90 into datahub-project:master Jun 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ingestion PR or Issue related to the ingestion of metadata
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants