-
Notifications
You must be signed in to change notification settings - Fork 4.8k
Adding Route model and APIs #88
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
pkg/router/etcdregistry.go
Outdated
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.
How about adding (err error) into func and then just call 'return' ?
e4f554a to
4733742
Compare
|
What's the state of this pull? |
69ef9d1 to
74a3d65
Compare
|
@smarterclayton can you do a quick review to see if this is good enough for an initial merge? |
pkg/route/api/register.go
Outdated
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 suggest removing unnecessary code instead of commenting it out.
7393a02 to
fac49d7
Compare
pkg/cmd/client/kubecfg.go
Outdated
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.
The gofmt on this is wrong
pkg/route/api/types.go
Outdated
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.
Call it ServiceName instead.
c213061 to
225fd54
Compare
|
I have made changes for the other feedback you provided. |
2d9d644 to
b791778
Compare
|
@smarterclayton made the change from Name to Host. Also, added a couple of extra parameters (Path and Scheme) and added validations and documentation. I tried to follow net/url URL struct fields for consistency and to be able to use that library for validations. |
pkg/route/registry/route/rest.go
Outdated
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.
If both params are of the same type, you can shorten this to List(selector, fields labels.Selector).
c370165 to
c81d8d6
Compare
|
Please squash |
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.
should be serviceName
|
Fix those remaining comments and squash |
The Route resource maps a frontend with a service to allow a router to route and/or load balance traffic to the service endpoints
c81d8d6 to
374a702
Compare
|
@smarterclayton done |
|
[merge] |
|
Origin Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/136/) (Image: devenv-fedora_200) |
|
Evaluated for origin up to 374a702 |
|
retroactive LGTM |
Merged by openshift-bot
Registry is a runtime dependency of the k8s broker - a store of definitions of service classes available via k8s broker.
No description provided.