-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
APP-driven catalog entity page with GitHub Actions integration & routes & tabs #2076
APP-driven catalog entity page with GitHub Actions integration & routes & tabs #2076
Conversation
packages/app/src/App.tsx
Outdated
path="/catalog/*" | ||
element={<CatalogPlugin EntityPage={EntityPage} />} | ||
/> | ||
{/* <Route path="/explore" element={<ExplorePlugin />} /> */} |
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.
Top-level registration of other plugins
@@ -89,7 +89,7 @@ const Root: FC<{}> = ({ children }) => ( | |||
<SidebarSearchField onSearch={handleSearch} /> | |||
<SidebarDivider /> | |||
{/* Global nav, not org-specific */} | |||
<SidebarItem icon={HomeIcon} to="./" text="Home" /> | |||
<SidebarItem icon={HomeIcon} to="/catalog" text="Home" /> |
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.
With this routing strategy catalog needs to move to /catalog
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.
which is probably fine, redirects can handle landing page changes
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.
Also here it's nicely clear that we could change it to "Home" and have any icon etc, so it's not the plugin that decided.
However, that leads to the question - when introducing a new plugin into your Backstage installation, will you HAVE to invent both paths, labels (such as in the sidebar), AND icons to wire them in? Or can we have some form of helper that just sorts it out based on a root route ref that holds a default icon and label and route, which all can be overridden? Kinda like <SidebarItem ref={catalogRef} />
or something
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.
Yup a routeRef
(ref
is reserved 😅) prop for the SidebarItem
is missing, we definitely want that.
We should preferably be able to generate a sidebar given a static config similar to this:
sidebar:
items:
- ref: catalog.catalog
- ref: scaffolder.create
- divider
- ref: explore.explore
- ref: techdocs.docs
}); | ||
routes.push({ | ||
path: '/*', | ||
element: <Navigate to="." />, |
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.
Catch-all to go to the nearest /
.
.
works, ..
works as well. /
in this case would've been resolved to the very top level /
as in localhost:3000/
}); | ||
}); | ||
const [matchedRoute] = | ||
matchRoutes(routes as RouteObject[], `/${params['*']}`) ?? []; |
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.
Since we are only inside /catalog/:kind/:optionalNameAndNamespace/*
route at this particular point, this params['*']
gets resolved into anything that comes after optionalNameAndNamespace
.
Example:
For the http://localhost:3000/catalog/Component/backstage/some/deep/route
- params['*'] === "some/deep/route"
const currentTab = tabs[selectedIndex]; | ||
const title = currentTab.label; | ||
|
||
const onTabChange = (index: number) => navigate(tabs[index].id.slice(1, -2)); |
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.
Removing leading /
and trailing /*
to enable built-in React Router v6 relative route resolution mechanism
path: string; | ||
exact?: boolean; | ||
}; | ||
EntityPageTabs.Tab = (_props: TabProps) => null; |
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.
Basically a fake component, which allows us to collect its props
|
||
const REDIRECT_DELAY = 2000; | ||
|
||
export const EntityContext = createContext<Entity>((null as any) as Entity); |
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 guy powers useEntity
hook
plugins/catalog/src/routes.ts
Outdated
title: 'Entity', | ||
}); | ||
export const entityRouteDefault = createRouteRef({ | ||
icon: NoIcon, | ||
path: '/catalog/:kind/:optionalNamespaceAndName', | ||
path: ':kind/:optionalNamespaceAndName', |
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.
Here is no leading /
- to enable <Link to={someRouteRef.path} />
being relative link.
For params - generatePath
from react-router
works
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.
The primary use-case for route-refs is linking into a plugin from other plugins or the app, doesn't this break that?
Btw, once we've settled the way we wanna do composition we want to have another look at route refs. Maybe they're not needed at all, but if they are we should convert the path
to a method that takes params. In this case the type of the route ref would be something like RouteRef<[kind: string, optionalNamespaceAndName: string]>
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.
The original RFC briefly hinted at this, but what I think we're really looking for here are immutable relative route refs. Something that lets you specify a route ref in relation to another route ref, where overrides on the second one would be reflected in the first.
@@ -170,6 +162,10 @@ export const WorkflowRunDetails = () => { | |||
} | |||
return ( | |||
<div className={classes.root}> | |||
<Breadcrumbs aria-label="breadcrumb"> | |||
<Link to="..">Workflow runs</Link> |
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.
..
means one level up, not to closest parent sadly. But I think this is good enough ;)
plugins/github-actions/src/plugin.ts
Outdated
router.addRoute(projectRouteRef, WorkflowRunsPage); | ||
router.addRoute(buildRouteRef, WorkflowRunDetailsPage); | ||
}, | ||
register() {}, |
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.
Maybe not needed anymore? 🤷♂️
switch (entity.kind) { | ||
case 'Component': | ||
return <ComponentEntity />; | ||
default: |
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.
Here we could fit in the APIEntityPage
, looking forward to these changes!
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.
sorry i only got partway ... will have to continue tomorrow
@@ -89,7 +89,7 @@ const Root: FC<{}> = ({ children }) => ( | |||
<SidebarSearchField onSearch={handleSearch} /> | |||
<SidebarDivider /> | |||
{/* Global nav, not org-specific */} | |||
<SidebarItem icon={HomeIcon} to="./" text="Home" /> | |||
<SidebarItem icon={HomeIcon} to="/catalog" text="Home" /> |
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.
which is probably fine, redirects can handle landing page changes
plugins/catalog/src/Router.tsx
Outdated
); | ||
}; | ||
|
||
export const CatalogPlugin = ({ EntityPage }) => ( |
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.
We should probably not name a component *Plugin
. The plugin should be the abstract notion of the entire plugin and all of the various things it exposes. CatalogPage
or CatalogRoot
or similar would make more sense.
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.
Yuuuuuuup. In general I think we can come up with a couple of suffixes for people to use depending on what is being exported. For example *Page
and *Card
.
I'm also thinking that all of these exported components will be wrapped by the plugin, so that we can inject a plugin context, pick up APIs, and ensure that they're all dynamically loaded.
packages/app/src/App.tsx
Outdated
<Routes> | ||
<Route | ||
path="/catalog/*" | ||
element={<CatalogPlugin EntityPage={EntityPage} />} |
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.
would be nice here if the EntityPage
prop was optional and had a simple but usable default implementation.
@@ -89,7 +89,7 @@ const Root: FC<{}> = ({ children }) => ( | |||
<SidebarSearchField onSearch={handleSearch} /> | |||
<SidebarDivider /> | |||
{/* Global nav, not org-specific */} | |||
<SidebarItem icon={HomeIcon} to="./" text="Home" /> | |||
<SidebarItem icon={HomeIcon} to="/catalog" text="Home" /> |
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.
Also here it's nicely clear that we could change it to "Home" and have any icon etc, so it's not the plugin that decided.
However, that leads to the question - when introducing a new plugin into your Backstage installation, will you HAVE to invent both paths, labels (such as in the sidebar), AND icons to wire them in? Or can we have some form of helper that just sorts it out based on a root route ref that holds a default icon and label and route, which all can be overridden? Kinda like <SidebarItem ref={catalogRef} />
or something
import { Grid } from '@material-ui/core'; | ||
|
||
const OverviewPage = () => { | ||
const entity = useEntity(); |
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.
thinking that this API should probably be const { entity } = useEntity();
, so we easily can tack more onto it - such as refresh()
or whatever that is needed
const entity = useEntity(); | ||
|
||
const isCIAvailable = [ | ||
entity!.metadata!.annotations?.[GITHUB_ACTIONS_ANNOTATION], |
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.
Should entity be nullable? Is that in like error conditions or something, is my first question then? Would be nice as a consumer to know it's always set, or an exception is thrown by the hook.
One more thing, is the exclamation mark necessary on metadata? I think we set it as required.
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.
Probably both things are fixable
|
||
return ( | ||
<Tabs> | ||
<Tabs.Tab title="Overview" path="/" exact> |
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.
the exact thing can be deprecated under react-router 6 right?
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 is not the react-router exact
though, it's part of the Tabs implementation
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.
Sure but maybe we want to change the tab implementation.
<Tabs.Tab title="CI/CD" path="/ci-cd"> | ||
{entity!.metadata!.annotations?.[GITHUB_ACTIONS_ANNOTATION] && ( | ||
<GitHubActionsPlugin entity={entity} /> | ||
)} |
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.
forgot circleci here :) yeah i know it's illustrative
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 think that the GitHubActionsPlugin
component should not be named Plugin
, as a plugin is the whole package, this component is the GithubActionsPage rather, IIUC.
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.
Or is this the actual way to hook in a full plugin?
|
||
export const ComponentEntity = () => { | ||
const entity = useEntity(); | ||
switch (entity.spec!.type) { |
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.
Question is if this switch is necessary - should we have to switch out / duplicate logic for choice of tabs, sidebar, etc per type? Maybe. Just wondering if it's the right "granularity".
)} | ||
</Tabs.Tab> | ||
)} | ||
<Tabs.Tab title="Docs" path="/docs"> |
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 guess we want to have something similar to isCIAvailable here but isDocsAvailable or something and check the techdocs ref annotation to only show the Docs tab on entities that actually have docs?
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.
Yes, point being - since it's JSX, one can introduce whatever logic is necessary
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.
ooor was that only for illustration purposes? xD
plugins/catalog/src/routes.ts
Outdated
title: 'Entity', | ||
}); | ||
export const entityRouteDefault = createRouteRef({ | ||
icon: NoIcon, | ||
path: '/catalog/:kind/:optionalNamespaceAndName', | ||
path: ':kind/:optionalNamespaceAndName', |
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.
The primary use-case for route-refs is linking into a plugin from other plugins or the app, doesn't this break that?
Btw, once we've settled the way we wanna do composition we want to have another look at route refs. Maybe they're not needed at all, but if they are we should convert the path
to a method that takes params. In this case the type of the route ref would be something like RouteRef<[kind: string, optionalNamespaceAndName: string]>
@@ -89,7 +89,7 @@ const Root: FC<{}> = ({ children }) => ( | |||
<SidebarSearchField onSearch={handleSearch} /> | |||
<SidebarDivider /> | |||
{/* Global nav, not org-specific */} | |||
<SidebarItem icon={HomeIcon} to="./" text="Home" /> | |||
<SidebarItem icon={HomeIcon} to="/catalog" text="Home" /> |
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.
Yup a routeRef
(ref
is reserved 😅) prop for the SidebarItem
is missing, we definitely want that.
We should preferably be able to generate a sidebar given a static config similar to this:
sidebar:
items:
- ref: catalog.catalog
- ref: scaffolder.create
- divider
- ref: explore.explore
- ref: techdocs.docs
</Tabs.Tab> | ||
{isCIAvailable && ( | ||
<Tabs.Tab title="CI/CD" path="/ci-cd"> | ||
{entity!.metadata!.annotations?.[GITHUB_ACTIONS_ANNOTATION] && ( |
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.
It will be tricky to generate an app based on static config if this kind of logic lives in the app. No need to fix as a part of this PR, but something we should look at after.
plugins/catalog/src/Router.tsx
Outdated
); | ||
}; | ||
|
||
export const CatalogPlugin = ({ EntityPage }) => ( |
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.
Yuuuuuuup. In general I think we can come up with a couple of suffixes for people to use depending on what is being exported. For example *Page
and *Card
.
I'm also thinking that all of these exported components will be wrapped by the plugin, so that we can inject a plugin context, pick up APIs, and ensure that they're all dynamically loaded.
…pp-catalog-tabs-routes-everything-is-connected
}; | ||
|
||
/** | ||
* Always going to return an entity, or throw an error if not a descendant of a EntityProvider. |
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.
@freben - that's what you wanted, right?
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.
Yep. But I'm not sure about the type in there, looks complex :)
…pp-catalog-tabs-routes-everything-is-connected
…pp-catalog-tabs-routes-everything-is-connected
docs/plugins/index.md
Outdated
@@ -26,3 +26,10 @@ This helps the community know what plugins are in development. | |||
|
|||
You can also use this process if you have an idea for a good plugin but you hope | |||
that someone else will pick up the work. | |||
|
|||
## Integrate into the catalog service |
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.
Service Catalog?
); | ||
|
||
const DefaultEntityPage = ({ entity }: { entity: Entity }) => ( | ||
<EntityPageLayout> |
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.
Try to move on top of EntityPage
const navigate = useNavigate(); | ||
|
||
React.Children.forEach(children, child => { | ||
if (!React.isValidElement(child)) { |
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.
Check if child type is actually Content
onChange={onTabChange} | ||
/> | ||
<Content> | ||
<Grid container spacing={3}> |
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.
Remove Grid
}, [errorApi, navigate, error, loading, entity]); | ||
|
||
if (!name) { | ||
navigate('/catalog'); |
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.
just /
|
||
type EntityLoadingStatus = { | ||
entity?: Entity; | ||
loading: boolean | null; |
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.
boolean
|
||
export const EntityContext = createContext<EntityLoadingStatus>({ | ||
entity: undefined as any, | ||
loading: null, |
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.
true in the beginning
plugins/catalog/src/routes.ts
Outdated
title: 'Entity', | ||
}); | ||
export const entityRouteDefault = createRouteRef({ | ||
icon: NoIcon, | ||
path: '/catalog/:kind/:optionalNamespaceAndName', | ||
path: ':kind/:optionalNamespaceAndName/*', |
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.
Remove /*
Leave old AppRoutes as well |
…pp-catalog-tabs-routes-everything-is-connected
…pp-catalog-tabs-routes-everything-is-connected
…pp-catalog-tabs-routes-everything-is-connected
Addresses the changes required after merging backstage#2076. The Sentry plugin now defines a router and the widget will be displayed as a tab.
Draft PR that solves most of the #1536 discovered problems 💪
![in](https://user-images.githubusercontent.com/5012901/90935948-950ed200-e404-11ea-8158-0eedae3fa764.gif)
Work towards #1982
TLDR
CatalogPage
which acceptsEntityPage
as a propEntityPage
is defined inside the APP, using helpers from plugin-catalog (Tabs, useEntity, etc)EntityPage
are now there in the APP, making it possible to compose/configure all you want out of it ;)TLDR (more details)