-
Notifications
You must be signed in to change notification settings - Fork 465
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
Reorganizes Docs and Adds HTTPRoute Resource Doc #463
Conversation
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.
Thanks for all your work on this! I realized part of the way through that I was commenting on things you only moved, not actually changed. I can move that to a follow up issue if you'd prefer.
docs-src/gateway.md
Outdated
A `Gateway` is 1:1 with the life cycle of the configuration of | ||
infrastructure. When a user creates a `Gateway`, some load balancing | ||
infrastructure is provisioned or configured | ||
(see below for details) by the `GatewayClass` controller. `Gateway` is the | ||
resource that triggers actions in this API. Other resources in this API are | ||
configuration snippets until a Gateway has been created to link the resources | ||
together. |
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 line length here feels a bit strange/inconsistent. I've been trying to wrap lines at 80 chars where possible though I know we haven't adopted any kind of formal standard there yet.
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 didn't update any of the existing content, but simply moved it as part of the refactor.
## Status | ||
|
||
Status defines the observed state of HTTPRoute. | ||
|
||
### RouteStatus | ||
|
||
RouteStatus defines the observed state that is required across all route types. |
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'm not sure how much value this has. We already have the API spec defined elsewhere in the docs so I don't think we need to cover every field unless we're adding new/more detailed or user friendly content.
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 thought it would be useful to include an example of what an HTTPRoute status looks like. The API docs for status do not provide an example. I can remove if you feel strongly about this comment.
[rfc-3986]: https://tools.ietf.org/html/rfc3986 | ||
[matches]: https://kubernetes-sigs.github.io/service-apis/spec/#networking.x-k8s.io/v1alpha1.HTTPRouteMatch | ||
[filters]: https://kubernetes-sigs.github.io/service-apis/spec/#networking.x-k8s.io/v1alpha1.HTTPRouteFilter | ||
[forwardto]: https://kubernetes-sigs.github.io/service-apis/spec/#networking.x-k8s.io/v1alpha1.HTTPRouteForwardTo |
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 actually thought this was going to link to the ForwardTo section of this doc. I think it's probably better to link to the API spec like you're doing here, just not what I expected. Maybe we should make these links target=_blank
since they're going to the spec?
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.
Maybe we should make these links target=_blank since they're going to the spec?
I'm not following what you mean by target=_blank
. Can you provide an example?
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.
Not important and maybe not even a good idea, would just default the link to open in a new tab/window depending on the browser.
55c74d6
to
bea1950
Compare
@robscott I addressed your comments either above or in commit |
Thanks! /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danehans, robscott The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
apis/v1alpha1/gateway_types.go
andapis/v1alpha1/httproute_types.go
: Fixes minor godoc errors.docs-src/httproute.md
: Adds docs for detailing theHTTPRoute
API type./assign @robscott @bowei @hbagdi
xref: #332