Skip to content

Amundsen Connector Fix #5267

Merged
pmbrull merged 5 commits intoopen-metadata:0.10.1from
ulixius9:issue-4243
Jun 2, 2022
Merged

Amundsen Connector Fix #5267
pmbrull merged 5 commits intoopen-metadata:0.10.1from
ulixius9:issue-4243

Conversation

@ulixius9
Copy link
Member

@ulixius9 ulixius9 commented Jun 2, 2022

Describe your changes :

Amundsen Connector Fix for 0.10.1

Type of change :

  • Bug fix

Checklist:

  • I have read the CONTRIBUTING document.
  • I have commented on my code, particularly in hard-to-understand areas.
  • I have added tests that prove my fix is effective or that my feature works.
  • All new and existing tests passed.

Reviewers

@open-metadata/ingestion

@ulixius9 ulixius9 requested a review from a team June 2, 2022 10:57
@ulixius9 ulixius9 temporarily deployed to cypress June 2, 2022 10:57 Inactive
@ulixius9 ulixius9 temporarily deployed to cypress June 2, 2022 10:57 Inactive
@ulixius9 ulixius9 temporarily deployed to cypress June 2, 2022 10:57 Inactive
@ulixius9 ulixius9 temporarily deployed to cypress June 2, 2022 10:57 Inactive
"delta": DatabaseServiceType.DeltaLake.value,
"salesforce": DatabaseServiceType.Salesforce.value,
"oracle": DatabaseServiceType.Oracle.value,
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

sorry, what's the goal of this?

@@ -223,7 +294,7 @@ def create_dashboard_entity(self, dashboard):
service_entity = get_dashboard_service_or_create(
Copy link
Collaborator

Choose a reason for hiding this comment

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

instead of get or create, we should just GET?

Copy link
Collaborator

Choose a reason for hiding this comment

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

if in the docs we specify that users should first create the services, then IMO we should:

  1. try to get the service
  2. if it is not there, throw an exception calling out that "service named XYZ of type ABC cannot be found"

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm logging an error in case of service not found, shall we throw error 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

mmh logging + ignoring the entity might be good enough!

Copy link
Member Author

Choose a reason for hiding this comment

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

yup

@ulixius9 ulixius9 temporarily deployed to cypress June 2, 2022 12:56 Inactive
@ulixius9 ulixius9 temporarily deployed to cypress June 2, 2022 12:56 Inactive
@ulixius9 ulixius9 temporarily deployed to cypress June 2, 2022 12:56 Inactive
@ulixius9 ulixius9 temporarily deployed to cypress June 2, 2022 12:56 Inactive
@pmbrull pmbrull merged commit 117e4f5 into open-metadata:0.10.1 Jun 2, 2022
@pmbrull
Copy link
Collaborator

pmbrull commented Jun 2, 2022

don't care about the sonar error here

@ulixius9 ulixius9 mentioned this pull request Jun 6, 2022
6 tasks
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.

2 participants