-
Notifications
You must be signed in to change notification settings - Fork 7k
Add app-backend plugin for serving app content #1986
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
Conversation
|
This is a great idea! +1 for having backend API routes prefixed with /api (as a separate change). |
We are already doing it, right now we configure it in our k8s ingress |
|
@Fox32 "We are already doing it, right now we configure it in our k8s ingress" Is there any chance this is open source? I spent some time on path based routing using k8s ingress and could not get it to work. I ended up using subdomains... |
We have something like: apiVersion: networking.k8s.io/v1beta1
kind: Ingress
metadata:
annotations:
cert-manager.io/cluster-issuer: letsencrypt
nginx.ingress.kubernetes.io/force-ssl-redirect: "true"
nginx.ingress.kubernetes.io/rewrite-target: /$1
name: backstage
spec:
rules:
- host: $(backstage_INGRESS).$(backstage_CLUSTER_DOMAIN)
http:
paths:
- backend:
serviceName: backstage-app
servicePort: http
path: /(.*)
- backend:
serviceName: backstage-backend
servicePort: http
path: /api/(catalog.*)
- backend:
serviceName: backstage-backend
servicePort: http
path: /api/(auth.*)
- backend:
serviceName: backstage-backend
servicePort: http
path: /api/(identity.*)
- backend:
serviceName: backstage-backend
servicePort: http
path: /api/(proxy.*) |
|
@Fox32 Thank you |
|
Huge +1 for this. |
|
rebased on master just because |
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.
instead of using a weird global, could we just add a requireImpl?: WhateverRequireTypeIs to RouterOptions, and have it default at runtime to the actual require? or, can we inject the actual app module 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.
I think we should add a package path resolve utility to backend-common, or look into if we can solve this by changing the behaviour of __dirname in the build. Either way I don't think we want the long term solution to be to pass the actual resolve function into the plugin.
| ); | ||
| await fs.writeFile(path, newContent, 'utf8'); | ||
| return; | ||
| } else if (content.includes('__APP_INJECTED_CONFIG_MARKER__')) { |
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.
Gonna be that guy and ask, can you explain again why it can happen that we need to re-inject/replace an existing 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.
In the docker-container case you have to re-create the container if you want to change any env vars, so there's no need to support re-injection there. With this packaging method someone might be running the backend outside of docker. In that case the backend might be started with different env var values, so we need to re-inject the config.
This adds an
app-backendplugin, allowing for a single deployment artifact by having frontend be bundled into and served by the backend.Config from
APP_CONFIGis injected and re-injected at startup, but otherwise the frontend config comes from the actual frontend build.This kinda makes me want to collect all plugin backends under an
api/router, so that we can have a proper 404 handler for API routes, and more straightforward load balancing. Right now you'll end up withindex.html, since we want HTML5 mode routing. That's not really related to this change though, as it'd be a thing for any deployment setup.