-
Notifications
You must be signed in to change notification settings - Fork 536
feat(tech-insights): add support for new frontend system #6802
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(tech-insights): add support for new frontend system #6802
Conversation
Changed Packages
|
83ff50c to
f746dc2
Compare
|
Hi! The Node 24.x CI failure appears to be a pre-existing infrastructure issue with The Node 22.x CI step passes all checks (tsc, lint, tests, api-reports). I noticed this was fixed in the announcements workspace via #6799 by updating better-sqlite3 from v9 to v12. The Should I include the better-sqlite3 upgrade in this PR, or should that be a separate PR? |
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.
Hi @OSfrog, Tech Insights is currently built against version 1.39.0 of Backstage, we made the New Frontend System adoption ready in 1.42.0 and this included several API changes. Even if this were to be merged you would find there's issues with it being used by other Adopters along with the New Frontend System. I would pause work on this PR and get Tech Insights version bumped to 1.46.x at which point you can rebase this PR and fix any of the breaking API changes.
|
@awanlin Thanks for the quick response. Shall I go ahead and create a separate PR for the version bump? |
eff418d to
4b13a60
Compare
|
@OSfrog please rebase, the version bump for Tech Insights to |
4b13a60 to
f28f801
Compare
|
Thanks @awanlin! I've rebased on main and addressed the breaking changes from the 1.46.x upgrade: New Frontend System changes addressed:
Backend changes:
All checks pass locally (tsc, lint, tests). Ready for review! |
vinzscam
left a comment
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!
workspaces/tech-insights/plugins/tech-insights/src/alpha/plugin.ts
Outdated
Show resolved
Hide resolved
workspaces/tech-insights/plugins/tech-insights/src/alpha/plugin.ts
Outdated
Show resolved
Hide resolved
workspaces/tech-insights/plugins/tech-insights/src/alpha/pages.tsx
Outdated
Show resolved
Hide resolved
workspaces/tech-insights/plugins/tech-insights/src/alpha/pages.tsx
Outdated
Show resolved
Hide resolved
workspaces/tech-insights/plugins/tech-insights/src/alpha/entityContent.tsx
Outdated
Show resolved
Hide resolved
workspaces/tech-insights/plugins/tech-insights/src/alpha/entityCards.tsx
Outdated
Show resolved
Hide resolved
workspaces/tech-insights/plugins/tech-insights/src/alpha/entityCards.tsx
Outdated
Show resolved
Hide resolved
workspaces/tech-insights/plugins/tech-insights/src/alpha/apis.ts
Outdated
Show resolved
Hide resolved
workspaces/tech-insights/plugins/tech-insights/src/alpha/apis.ts
Outdated
Show resolved
Hide resolved
ae3b322 to
619985a
Compare
|
@vinzscam requested changes are made, is there anything else needed to merge this? We are being blocked by this PR 🙇 |
workspaces/tech-insights/plugins/tech-insights/src/alpha/apis.ts
Outdated
Show resolved
Hide resolved
| * | ||
| * @alpha | ||
| */ | ||
| export const entityTechInsightsScorecardCard = EntityCardBlueprint.make({ |
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.
Two things to consider, even as follow-ups:
- End users are likely going to want some customization over the component, so I see this being converted to a
makeWithOverridesblueprint. - Should we filter out any entity kinds by default? Right now this will appear on all entity kinds by 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.
Good points! What do you think about filtering out Location and Domain entities? I think the rest are fine.
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 still think these are good things to consider, but I'm now doubting doing it on this first release. I'm not fully understanding how consumers configure how and where this component appears in the new frontend system.
619985a to
5889d5d
Compare
Added alpha exports for the new Backstage frontend system: - techInsightsPlugin: Main plugin for the new frontend system - techInsightsApi: API extension using ApiBlueprint - techInsightsScorecardPage: Page extension at /tech-insights - entityTechInsightsScorecardContent: Entity content tab - entityTechInsightsScorecardCard: Entity overview card Signed-off-by: Tommy Le <[email protected]>
5889d5d to
5bfac5d
Compare
kurtaking
left a comment
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.
Noticed a few more things
workspaces/tech-insights/plugins/tech-insights/src/alpha/index.ts
Outdated
Show resolved
Hide resolved
| * | ||
| * @alpha | ||
| */ | ||
| export const entityTechInsightsScorecardCard = EntityCardBlueprint.make({ |
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 still think these are good things to consider, but I'm now doubting doing it on this first release. I'm not fully understanding how consumers configure how and where this component appears in the new frontend system.
Signed-off-by: Tommy Le <[email protected]>
kurtaking
left a comment
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've tested installing locally with a new instance of Backstage on the new frontend system and think we are good to go with initial release here.
Thanks for the contribution!
|
@vinzscam, this is still pending requested changes from you which have been addressed. Can we either dismiss your review or have you provide approval? Thanks! |
|
I've opened #6945 as a follow-up to add a nav item. |
Hey, I just made a Pull Request!
Added alpha exports for the new Backstage frontend system:
Usage:
✔️ Checklist
Signed-off-byline in the message. (more info)