-
Notifications
You must be signed in to change notification settings - Fork 3k
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(graphql): enabling graphql for data platform instance aspects #7177
feat(graphql): enabling graphql for data platform instance aspects #7177
Conversation
I've been following instructions here https://datahubproject.io/docs/datahub-graphql-core/#extending-the-graph Although this PR is about adding the aspects to the GraphQL interface, I would like to check the Data Platform Instance entity is searchable. While the instructions mention this related to that:
I couldn't find where to add the DataPlatformInstance, so I cannot confirm in that code whether it is searchable or not. Likely the code has evolved and those instructions are not up-to-date. |
hey @sgomezvillamor thanks for the PR! gonna give this a look through now :) |
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.
Nice! this is looking great. I have a few suggestions / changes but once that's settled I think this is ready to rumble!
...n/java/com/linkedin/datahub/graphql/types/dataplatforminstance/DataPlatformInstanceType.java
Outdated
Show resolved
Hide resolved
...n/java/com/linkedin/datahub/graphql/types/dataplatforminstance/DataPlatformInstanceType.java
Show resolved
Hide resolved
.../linkedin/datahub/graphql/types/dataplatforminstance/mappers/DataPlatformInstanceMapper.java
Outdated
Show resolved
Hide resolved
.../linkedin/datahub/graphql/types/dataplatforminstance/mappers/DataPlatformInstanceMapper.java
Outdated
Show resolved
Hide resolved
Thanks @chriscollins3456 for the review. Here my update:
|
@sgomezvillamor sounds good! let me know if you have any questions - I'm happy to provide some more examples or collaborate some more on this. |
Hi @chriscollins3456 I have addressed the comment about the code style with the mapperHelper. May you have a look? Let me know if there is any other update that you would like to include in this PR. From my side, the question raised here #7177 (comment) is still open to me. With the updates done here in this PR, is the Data Platform Instance searchable by its aspects? Thanks! |
Sorry I didn't even notice that question above! yeah so what you need to do is update |
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.
@sgomezvillamor This is looking great for what you have! I know you might want to make one last change and add to SEARCHABLE_ENTITY_TYPES
and AUTO_COMPLETE_ENTITY_TYPES
as well, just let me know when you push that up and we can get this guy in there
Hi @chriscollins3456 ... here it is, last but not least f975307 :-) |
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.
@sgomezvillamor actually this last commit does cause concern..
If we add it to SEARCHABLE_ENTITY_TYPES
and AUTO_COMPLETE_ENTITY_TYPES
then data platform instances will start showing up in search and auto-complete results.
however, we don't have search result cards or a profile page for data platform instances built on the frontend yet, so I think this will definitely cause problems.
so before we add those two things in SearchUtils.java
we need to build out the frontend pieces for data platform instances!
… and autocompleted" This reverts commit f975307.
Good point on search and autocompletion @chriscollins3456 ! I have reverted that commit and explicitly mentioned in the PR description the features not covered in the scope of this PR (updates, search+autocomplete, UI elements). |
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.
alright nice! thanks so much for this. Excited for this feature development to keep on rolling!
Also sorry about the confusion around adding to seachable entity types and auto-complete - I wasn't thinking it fully through until you had made the change
Thanks to you for your speedy and responsive support! |
By the way, I cannot merge, so all yours, folks! |
This continues the work started here #5728 and so it enables fetching Data Platform Instance aspects via GraphQL.
Not in the scope of this PR and so still pending:
Checklist