Add ingress domain #139
Add ingress domain #139
Conversation
openshift-ci-robot
added
the
size/XS
config/v1/types_ingress.go
Outdated
|
|
||
| type IngressSpec struct { | ||
| // default suffix. It goes here or it gets removed from server | ||
| // Domain specifies the default subdomain for routes. |
There was a problem hiding this comment.
lower "d" in API docs to match the json string.
There was a problem hiding this comment.
Also is it common knowledge for OpenShift users that domain for ingress means subdomain?
There was a problem hiding this comment.
Should be. I went with "domain" for consistency with "base domain" (which is as much a subdomain as is the ingress domain). We can explain in documentation how the ingress domain is used to compose the route domain.
|
@ironcladlou confirm that you expect and want the domain to be user changeable. That's a significant change. |
|
According to #124 (comment)
I didn't see anything about using spec for mutable fields and status for immutable fields. Is that documented? |
It's about user specifiable or not. The kube convention is that user specifiable fields are in spec, non-user specifiable ones are in status. |
|
It's user-specifiable, but only once (during installation), and it's not anyone-changeable (neither users nor operators should be changing it). Documentation could be clearer. I see your point though and will update the PR shortly. |
Miciah
force-pushed
the
add-ingress-domain
branch
from
f30cb17 to
a8311e2
Compare
|
Updated to move the new field from spec to status. |
|
/approve @ironcladlou your API. If you're happy with the field name and doc, you can lgtm. Think of this as your doc that will explain to an admin what it does and to developers how to use it. /assign @ironcladlou |
openshift-ci-robot
added
the
approved
config/v1/types_ingress.go
Outdated
| } | ||
|
|
||
| type IngressStatus struct { | ||
| // domain specifies the default subdomain that will be used to compose |
There was a problem hiding this comment.
How about:
domain is used to generate the hostname for a route when none is specified. The pattern that is used is: routename-projectname.domain.
|
@deads2k it feels really weird for something to read its state from status not spec. I would argue that it is mutable, but doing so ought to be a rare event. So I'd lean for spec. Thoughts? |
|
I was wrong, it's mutable after all. Back to spec it goes! |
* config/v1/types_ingress.go (IngressSpec): Add Domain field. Related to NE-100. https://jira.coreos.com/browse/NE-100
Miciah
force-pushed
the
add-ingress-domain
branch
from
a8311e2 to
2ba7723
Compare
|
And with the latest push, |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, ironcladlou, Miciah The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
7 participants
config/v1/types_ingress.go(IngressSpec): AddDomainfield.Related to NE-100.