-
Notifications
You must be signed in to change notification settings - Fork 3.2k
feat(ingestion-ui) Display ingestion sources in UI more dynamically #5789
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(ingestion-ui) Display ingestion sources in UI more dynamically #5789
Conversation
@@ -63,7 +64,8 @@ const CliBadge = styled.span` | |||
`; | |||
|
|||
export function TypeColumn(type: string, record: any) { | |||
const iconUrl = sourceTypeToIconUrl(type); | |||
let iconUrl = useGetSourceLogoUrl(record.platformUrn || ''); |
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.
hmm--- this seems overly complex. why aren't we just fetching source.platform.logoUrl in the initial data pull?
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.
ugh i know. so it turns out that I don't believe that these ingestion sources often actually have a platform associated with them, so it's usually null (idk how the platform gets set on these sources), so yes I agree it would make sense to just get the logoUrl. The only thing is that here I'm trying to short-circuit and use the logo we have in memory in the react bundle before we fetch to get the image from where we host it on github somewhere.
So really this code block is almost always (if not always always?) going to return null and go to the next line to get the icon purely based on what we have in react.
[MODE_URN]: modeLogo, | ||
}; | ||
|
||
export const SOURCE_TO_SOURCE_URN = { |
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.
this feels wrong to me 🤔 if we are already encoding this mapping in mysql why do we need to recreate it in react?
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.
after some help, realized that the urn is just urn:li:dataPlatform:<name>
so this is no longer necessary anyways!
This is a first step PR to get displaying available ingestion sources in the UI dynamic. Right now everything is done on the frontend with a handwritten json file. However, that's just a stub until we eventually build out the ability to fetch this data dynamically to always keep the UI up to date.
Instead of relying on a file for each source with its configs, define these configs in a mock json file. Then create a mapper from source urn to source logo for displaying in the UI. Also creates a custom hook to check for this logo in the map in memory and if it's not there, fetch it from the backend.
Finally this updates our docs to add one final step to the guide on adding a new ingestion source - what to do in order to get that source to show up in the frontend.
This PR also adds 10 new sources:
The next PR will be deleting all of the source config files and moving any dependencies on them. I thought it was cleaner to do this + that in separate PRs.
Checklist