-
Notifications
You must be signed in to change notification settings - Fork 160
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(app): dynamic Techdocs addons #2238
feat(app): dynamic Techdocs addons #2238
Conversation
5187cf2
to
fdd354b
Compare
The image is available at: |
The image is available at: |
The image is available at: |
/cc @gashcrumb can you take a look if this approach is alright? There is a bug with |
88c2f2a
to
c3117d1
Compare
ReportIssue bug is fixed. |
The image is available at: |
c3117d1
to
0abe50e
Compare
ed2d797
to
6a442d3
Compare
The image is available at: |
Removed previously added |
The image is available at: |
/cc @davidfestal |
Signed-off-by: Dominika Zemanovicova <[email protected]>
Signed-off-by: Dominika Zemanovicova <[email protected]>
Signed-off-by: Dominika Zemanovicova <[email protected]>
Signed-off-by: Dominika Zemanovicova <[email protected]>
Signed-off-by: Dominika Zemanovicova <[email protected]>
Signed-off-by: Dominika Zemanovicova <[email protected]>
Signed-off-by: Dominika Zemanovicova <[email protected]>
Signed-off-by: Dominika Zemanovicova <[email protected]>
Signed-off-by: Dominika Zemanovicova <[email protected]>
Signed-off-by: Dominika Zemanovicova <[email protected]>
Signed-off-by: Dominika Zemanovicova <[email protected]>
Signed-off-by: Dominika Zemanovicova <[email protected]>
Signed-off-by: Dominika Zemanovicova <[email protected]>
Signed-off-by: Dominika Zemanovicova <[email protected]>
Signed-off-by: Dominika Zemanovicova <[email protected]>
daaba49
to
47fabd8
Compare
/retest |
The image is available at: |
For some reason I didn't understand yet, the latest version didn't worked on a cluster. Absolutely no dynamic plugin is loaded. We need to check if its this PR or a general issue. But to be save I mark this as /hold |
@christoph-jerolimov I was able to make it work on a cluster. Wonder if there was some other issue that was popping up for you? |
@@ -471,6 +471,9 @@ dynamicPlugins: | |||
props: | |||
name: Documentation | |||
icon: docs | |||
backstage.plugin-techdocs-module-addons-contrib: |
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 seen both a .
and a -
used as the org/package separator. I think we should consider that inconsistency here confuses users, and I wonder if we shouldn't just stick with using -
. I'll have a 2nd comment on this in the plugin's scalprum
config :-)
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 would maybe let it be this way for now, but we can do a following bulk update for all the plugins that have it this way.
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, I don't know if there's any formal convention here so maybe one needs to be established.
"dist", | ||
"dist-scalprum" | ||
], | ||
"scalprum": { |
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.
Is it necessary to add this config? Without it I believe the scalprum name would be backstage-plugin-techdocs-module-addons-contrib
which I think actually is less confusing as it's taken from the package.json name. I just worry these slight differences here and there confuse users, I recently had a conversation around this field, the package name and what is used in the configuration. Maybe not something to fix right now but to consider very soon.
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.
Yeah, not necessary. I have seen both versions being used, it would be great to unify this.
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 did a quick local test using the PR image and could see the add-ons installed, very nice! A couple comments but nothing that needs to be addressed in this PR.
I've tested it now again on a new cluster and couldn't reproduce my earlier issue. 🤷♂️ /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: christoph-jerolimov, gashcrumb The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
56cbf89
into
redhat-developer:main
Description
This change adds support for dynamically loading Techdocs Addons. It creates a wrapper for techdocs-module-addons-contrib which contains techdocs extensions:
A plugin can export a Techdocs extension like:
Use new configuration option:
Which issue(s) does this PR fix
PR acceptance criteria
Please make sure that the following steps are complete:
How to test changes / Special notes to the reviewer
You can add to
app-config.yaml
this config:2025-01-24.12-39-47.mp4