-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Serve web console from same port as API by default and allow configuring console's context root #1458
Conversation
@@ -273,6 +270,54 @@ func initAPIVersionRoute(root *restful.WebService, version string) { | |||
Consumes(restful.MIME_JSON)) | |||
} | |||
|
|||
// If we know the location of the asset server, redirect to it when / is requested | |||
// and the Accept header supports text/html | |||
func assetServerRedirect(handler http.Handler, assetPublicURL string) http.Handler { |
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 there a better place to put this?
implemented all review feedback except for the ongoing discussion about CORS [test] |
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_openshift3/1472/) |
func indexAPIPaths(handler http.Handler) http.Handler { | ||
return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { | ||
if req.URL.Path == "/" { | ||
object := api.RootPaths{Paths: []string{ |
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.
Add a TODO to build these paths in a non-hardcoded 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.
Integration test for root redirect with "accept: text/html" and json otherwise?
https://gist.github.com/liggitt/602ad5b5cfab6d156c90
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.
Also, add a TODO in the integration test file to add a test when the asset config is nil, once David's changes to let you manipulate the config are available
… or as its own server on a separate port
… to asset server or dump of api paths when / is requested.
@liggitt squashed down to two commits, your initial changes and then all my follow-up changes, ready for a final glance-over before merge |
[merge] |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/1310/) (Image: devenv-fedora_1142) |
Evaluated for origin up to 22045ca |
impacts the sample app readme, right? |
@bparees you are correct, I was about to add info about the console to the main readme, will fix that at the same time |
Merged by openshift-bot
func assetServerRedirect(handler http.Handler, assetPublicURL string) http.Handler { | ||
return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { | ||
accept := req.Header.Get("Accept") | ||
if req.URL.Path == "/" && strings.Contains(accept, "text/html") { |
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.
@jwforres I'd say this might be error prone, looking at this http spec, which allows you to specify the accept header in quite sophisticated way. Eg. if I'd send Accept: text/html; q=0.2, application/json
you'd classify this as web request, whereas actually text/htm
l option has q=0.2
and application/json
has q=1
which means is the preferred one. Other example would be that there is possibility the header will contain */*
which may go with text/html
but will not with this. I couldn't find any go lib to cover this, but here implementation of this in python. I'm willing to help with this or provide more details on it as I've spent quite some time digging into this some time ago. On the other hand maybe something has changed in the mean time.
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.
We're aware this isn't fully spec compliant. Feel free to add a todo or open a pull request, but it's working well enough for now (browsers get redirected, curl and requests that don't accept text/html explicitly get the JSON.
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 created an issue to track this
#1480
On Thu, Mar 26, 2015 at 11:40 PM Jordan Liggitt notifications@github.com
wrote:
In pkg/cmd/server/origin/master.go
#1458 (comment):@@ -273,6 +271,55 @@ func initAPIVersionRoute(root *restful.WebService, version string) {
Consumes(restful.MIME_JSON))
}+// If we know the location of the asset server, redirect to it when / is requested
+// and the Accept header supports text/html
+func assetServerRedirect(handler http.Handler, assetPublicURL string) http.Handler {
- return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
accept := req.Header.Get("Accept")
if req.URL.Path == "/" && strings.Contains(accept, "text/html") {
We're aware this isn't fully spec compliant. Feel free to add a todo or
open a pull request, but it's working well enough for now (browsers get
redirected, curl and requests that don't accept text/html explicitly get
the JSON.—
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/1458/files#r27263311.
…service-catalog/' changes from aa27078754..dabde2eb85 dabde2eb85 origin build: add origin tooling b70c076 Reorder class and plan creation; test plan conflict handling (openshift#1459) 4bea012 Use versioned client APIs (openshift#1458) ff4af30 clean up logic for 410 gone deprovision poll (openshift#1452) 3fddf27 clean up logic and fix message for failed poll (openshift#1451) 40926cd Fix typo from openshift#1354 (openshift#1456) ff86ef2 Delete removed serviceplans when they have no instances left (openshift#1444) 8411a16 tweak binding setAndUpdateOrphanMitigation function (openshift#1448) ce28252 Combine apiserver and controller-manager into a single service-catalog image (openshift#1343) 7bbc8ee Check service class / plan before allowing provisioning or plan changes. (openshift#1439) baf28de Create listers before adding event handlers in controller (openshift#1446) 294157d remove setServiceBindingCondition dependency on controller (openshift#1441) 118a0f7 Fix typo in validation (openshift#1447) 117bfbd clean up error logging (openshift#1443) dff470f Move "External" around in some resource names/properties (openshift#1354) 0885edb Adding expectedGot function and using it. (openshift#1440) a7d582e Pretty controller broker (openshift#1442) c5edfaf Set apimachinery build variables with semver info (openshift#1429) 0e90d82 Add a pretty formatter for ClusterService[Class|Plan] (openshift#1408) fb874df Remove deprecated basic auth config support (openshift#1431) f4cd181 Migrate to metav1 methods for manipulating controllerRefs (openshift#1433) 96b286e Make service/plan reference fields on instance spec selectable (openshift#1422) 33f2b04 First example using the pretty context builder. (openshift#1403) 7852917 Stop using corev1.ObjectReference and corev1.LocalObjectReference (openshift#1417) fcf9480 Add tests for plan updates (openshift#1412) 819332e Add root CAs (openshift#1419) b49a76a Clean Makefile a little (openshift#1399) d681da0 Use a separate etcd prefix for each integration test to keep tests isolated (openshift#1415) 314a622 Wire etcd prefix to storage and call complete with options (openshift#1394) REVERT: aa27078754 origin build: add origin tooling git-subtree-dir: cmd/service-catalog/go/src/github.com/kubernetes-incubator/service-catalog git-subtree-split: dabde2eb859b5e31e97c01a704561fc27e1848b2
…service-catalog/' changes from aa27078754..510060232e 510060232e origin build: add origin tooling de45e94 v0.1.0 chart changes (openshift#1468) 0bb9982 Modify Makefile to only specify ldflags once (openshift#1471) 5d6afac Fixes openshift#735: Add repo-sync script for charts (openshift#1453) 630f13f fix lingering unversioned client API (openshift#1466) 6f49128 Fix several logging errors (openshift#1464) 2aece61 Delete removed serviceClasses when they have no instances left (openshift#1450) 179d302 Uncommenting UID field after updating to k8s 1.8 (openshift#1457) b70c076 Reorder class and plan creation; test plan conflict handling (openshift#1459) 4bea012 Use versioned client APIs (openshift#1458) ff4af30 clean up logic for 410 gone deprovision poll (openshift#1452) 3fddf27 clean up logic and fix message for failed poll (openshift#1451) 40926cd Fix typo from openshift#1354 (openshift#1456) ff86ef2 Delete removed serviceplans when they have no instances left (openshift#1444) 8411a16 tweak binding setAndUpdateOrphanMitigation function (openshift#1448) ce28252 Combine apiserver and controller-manager into a single service-catalog image (openshift#1343) 7bbc8ee Check service class / plan before allowing provisioning or plan changes. (openshift#1439) baf28de Create listers before adding event handlers in controller (openshift#1446) 294157d remove setServiceBindingCondition dependency on controller (openshift#1441) 118a0f7 Fix typo in validation (openshift#1447) 117bfbd clean up error logging (openshift#1443) dff470f Move "External" around in some resource names/properties (openshift#1354) 0885edb Adding expectedGot function and using it. (openshift#1440) a7d582e Pretty controller broker (openshift#1442) c5edfaf Set apimachinery build variables with semver info (openshift#1429) 0e90d82 Add a pretty formatter for ClusterService[Class|Plan] (openshift#1408) fb874df Remove deprecated basic auth config support (openshift#1431) f4cd181 Migrate to metav1 methods for manipulating controllerRefs (openshift#1433) 96b286e Make service/plan reference fields on instance spec selectable (openshift#1422) 33f2b04 First example using the pretty context builder. (openshift#1403) 7852917 Stop using corev1.ObjectReference and corev1.LocalObjectReference (openshift#1417) fcf9480 Add tests for plan updates (openshift#1412) 819332e Add root CAs (openshift#1419) b49a76a Clean Makefile a little (openshift#1399) d681da0 Use a separate etcd prefix for each integration test to keep tests isolated (openshift#1415) 314a622 Wire etcd prefix to storage and call complete with options (openshift#1394) REVERT: aa27078754 origin build: add origin tooling git-subtree-dir: cmd/service-catalog/go/src/github.com/kubernetes-incubator/service-catalog git-subtree-split: 510060232e54eb64b294213bb5d7847e169a2fac
…21449-upstream-ose-enterprise-3.11 Automated cherry pick of openshift#21449 on enterprise-3.11
No description provided.