-
Notifications
You must be signed in to change notification settings - Fork 27
Conversation
🚀 Deployed Preview: http://konveyor-virt-ui-pr-214-preview.surge.sh ✨ |
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.
@gildub aside from the comments below, the only thing you were missing was to change the UI code so it uses those new /cluster-api
, /inventory-api
and /inventory-payload-api
URLs instead of the ones in VIRT_META!
I made this change locally and the proxy works flawlessly!
I opened a PR against your branch to make this change: https://github.com/gildub/virt-ui/pull/1
deploy/server.js
Outdated
@@ -69,7 +71,59 @@ if (process.env['DATA_SOURCE'] !== 'mock') { | |||
}); | |||
} | |||
|
|||
app.get('*', (req, res) => { | |||
let clusterApiProxyOptions = { | |||
target: virtMeta.clusterApi, |
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.
For these targets, we should have conditions where in prod we use the internal endpoints described in #152 (comment) instead of the values from virtMeta
.
e.g.
let clusterApiProxyOptions = {
target:
process.env['NODE_ENV'] !== 'development'
? 'https://api.openshift-apiserver.svc.cluster.local/'
: virtMeta.clusterApi,
[...]
};
}; | ||
|
||
if (process.env['NODE_ENV'] === 'development') { | ||
clusterApiProxyOptions = { |
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 I suppose you could just replace the target
properties here since you already have a condition for dev mode.
To save time I'll go ahead and address the other feedback in https://github.com/gildub/virt-ui/pull/1. |
Ok, https://github.com/gildub/virt-ui/pull/1 is ready and addresses all of the above. |
We should probably also remove the |
@mturley, great, thanks. |
Switch UI to use proxied API paths, switch proxy to use internal `.local` URLs, disable HTTPS
Whoops! Thanks for the fix. |
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.
LGTM!
Resolves #152