Skip to content
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(backend): add service factories by default #1881

Merged
merged 1 commit into from
Nov 19, 2024

Conversation

gashcrumb
Copy link
Member

@gashcrumb gashcrumb commented Nov 5, 2024

Description

This change explicitely adds the default service factories to the backend statically to prevent dynamic plugins from being able to override them by default. It's possible to override each statically added service factory via an environment variable derived from the service factory ID. So for example to add a "core.rootHttpService" service factory configuration from a dynamic plugin, set ENABLE_CORE_ROOTHTTPROUTER_OVERRIDE to "true". This change also adds a logger to the backend main. Finally, a unit test has been added that checks the installed backend-defaults value for the defaultServiceFactories list against what this change adds to catch future regressions.

Which issue(s) does this PR fix

PR acceptance criteria

Please make sure that the following steps are complete:

  • GitHub Actions are completed and successful
  • Unit Tests are updated and passing
  • E2E Tests are updated and passing
  • Documentation is updated if necessary (requirement for new features)
  • Add a screenshot if the change is UX/UI related

How to test changes / Special notes to the reviewer

It's enough to just check that this doesn't break anything, however an example exists here that has a plugin that can be used to try this out.

@gashcrumb gashcrumb requested a review from a team as a code owner November 5, 2024 11:33
Copy link
Contributor

github-actions bot commented Nov 5, 2024

The image is available at: quay.io/janus-idp/backstage-showcase:pr-1881!

Copy link
Contributor

github-actions bot commented Nov 6, 2024

Copy link
Contributor

github-actions bot commented Nov 6, 2024

@PatAKnight
Copy link
Member

/test e2e-tests

Copy link
Member

@davidfestal davidfestal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we do something more general than that, for all the default services added in the createdBackend()method ?
With environment variables like:
ENABLE_OVERRIDING_SERVICE_ (serviceFactoryName being the service factory name mentioned in the list of default services: https://github.com/backstage/backstage/blob/master/packages/backend-defaults/src/CreateBackend.ts#L37)

@gashcrumb
Copy link
Member Author

Shouldn't we do something more general than that, for all the default services added in the createdBackend()method ? With environment variables like: ENABLE_OVERRIDING_SERVICE_ (serviceFactoryName being the service factory name mentioned in the list of default services: backstage/backstage@master/packages/backend-defaults/src/CreateBackend.ts#L37)

I can look at doing that, certainly. I didn't see the same customization options offered for some of the other services when I was looking through them in preparation for this, so I opted to keep the scope for this much more narrow. But I'll take another look. Thanks!

@gashcrumb gashcrumb marked this pull request as draft November 7, 2024 12:09
@gashcrumb gashcrumb changed the title feat(backend): add default root http router config feat(backend): add service factories by default Nov 7, 2024
@davidfestal
Copy link
Member

I like the last commit: 4a76ffa

Would it be possible to add a unit test that would simply extract the upstream source file based on the backstage.json of the showcase, and check that the service factories are the same ?

Copy link
Contributor

github-actions bot commented Nov 7, 2024

@gashcrumb
Copy link
Member Author

I like the last commit: 4a76ffa

Cool!

Would it be possible to add a unit test that would simply extract the upstream source file based on the backstage.json of the showcase, and check that the service factories are the same ?

yeah good call, lemme look into that.

@gashcrumb gashcrumb marked this pull request as ready for review November 7, 2024 17:47
@openshift-ci openshift-ci bot requested a review from dzemanov November 7, 2024 17:47
Copy link
Contributor

github-actions bot commented Nov 7, 2024

Copy link
Contributor

github-actions bot commented Nov 8, 2024

Copy link
Contributor

github-actions bot commented Nov 8, 2024

Copy link
Contributor

Copy link
Contributor

@gashcrumb gashcrumb force-pushed the RHIDP-4786 branch 2 times, most recently from 21b17fa to 29f4885 Compare November 18, 2024 15:10
Copy link
Contributor

Copy link
Contributor

@gashcrumb
Copy link
Member Author

@subhashkhileri something seems amiss with the test env, I wonder if the persistent store with the dynamic plugins isn't cleaned up between the non-rbac and rbac tests. Looking at the failed tests, I see the rbac showcase instance loaded the segment plugin:

"level":"\u001b[32minfo\u001b[39m","message":"Loaded dynamic frontend plugin 'janus-idp-backstage-plugin-analytics-provider-segment' from 'file:///opt/app-root/src/dynamic-plugins-root/janus-idp-backstage-plugin-analytics-provider-segment-1.10.0' ","plugin":"scalprum","service":"backstage","timestamp":"2024-11-19 05:26:00"}

But I can see it's disabled here. Is there a chance maybe the PV holding the dynamic-plugins-root is getting reused or could this be a symptom of trying to run tests concurrently?

@gashcrumb gashcrumb force-pushed the RHIDP-4786 branch 3 times, most recently from efe2f17 to c59af97 Compare November 19, 2024 12:23
Copy link
Contributor

@subhashkhileri
Copy link
Member

/test e2e-tests

@davidfestal
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Nov 19, 2024
Copy link

openshift-ci bot commented Nov 19, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidfestal

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

This change explicitely adds the default service factories to the
backend statically to prevent dynamic plugins from being able to
override them by default.  It's possible to override each statically
added service factory via an environment variable derived from the
service factory ID.  So for example to add a "core.rootHttpService"
service factory configuration from a dynamic plugin, set
ENABLE_CORE_ROOTHTTPSERVICE_OVERRIDE to "true".  This change also adds a
logger to the backend main.  Finally, a unit test has been added that
checks the installed backend-defaults value for the
defaultServiceFactories list against what this change adds to catch
future regressions.

Signed-off-by: Stan Lewis <[email protected]>
Copy link

openshift-ci bot commented Nov 19, 2024

New changes are detected. LGTM label has been removed.

@gashcrumb gashcrumb added the lgtm label Nov 19, 2024
Copy link
Contributor

@openshift-merge-bot openshift-merge-bot bot merged commit 362da9d into redhat-developer:main Nov 19, 2024
14 checks passed
teknaS47 pushed a commit to teknaS47/backstage-showcase that referenced this pull request Nov 28, 2024
This change explicitely adds the default service factories to the
backend statically to prevent dynamic plugins from being able to
override them by default.  It's possible to override each statically
added service factory via an environment variable derived from the
service factory ID.  So for example to add a "core.rootHttpService"
service factory configuration from a dynamic plugin, set
ENABLE_CORE_ROOTHTTPSERVICE_OVERRIDE to "true".  This change also adds a
logger to the backend main.  Finally, a unit test has been added that
checks the installed backend-defaults value for the
defaultServiceFactories list against what this change adds to catch
future regressions.

Signed-off-by: Stan Lewis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants