-
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
write a very simple getting started guide #427
write a very simple getting started guide #427
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 your work on this!
kubectl kustomize "github.com/kubernetes-sigs/service-apis/config/crd?ref=master" \ | ||
| kubectl apply -f - |
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 good way to do this without kustomize? It seems like we're not really using kustomize for anything. (This does not need to block this PR).
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 do this in our Makefile too: https://github.com/kubernetes-sigs/service-apis/blob/master/Makefile#L54
Do note that the user doesn't need to install kustomize
. It is included as a plugin inside kubectl
itself. Users need to make sure that their kubectl
version is not an ancient one, which again is expected.
docs-src/getting-started.md
Outdated
|
||
## Install an implementation | ||
|
||
Service APIs is a set of specification that multiple implementations confirm to. |
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.
This doesn't feel quite right. Maybe "Multiple projects implement the APIs defined by this project."
docs-src/getting-started.md
Outdated
|
||
In this example, we are installing three resources: | ||
|
||
- An `acme-lb` GatewayClass which is being managed by a `acme.io/gateway-controller` controller running in the cluster. |
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.
Nit: This line feels too long.
docs-src/getting-started.md
Outdated
|
||
- Finally, we have an HTTPRoute resource which is attached to the above Gateway | ||
and has two rules: | ||
- All requests with Path beginning with `/bar` are forwarded to my-service1 |
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.
Nit: I'm not sure path needs to be capitalized here. I think it could use an a
before it though.
docs-src/getting-started.md
Outdated
and has two rules: | ||
- All requests with Path beginning with `/bar` are forwarded to my-service1 | ||
Kubernetes Service. | ||
- All requests with Path beginning with `/some/thing` AND have an HTTP header |
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.
Same comment as above
docs-src/getting-started.md
Outdated
Service APIs is a collection of Kubernetes Custom Resource Definitions that need | ||
to be installed into any Kubernetes cluster. |
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 "This project provides a collection of Custom Resource Definitions that can be installed into any Kubernetes 1.16+ cluster."
Depends on #425 |
8502d40
to
f6a78f3
Compare
|
/retest |
@hbagdi Our verify output is super confusing, in this case it looks like another run of
|
54b393f
to
a50ca52
Compare
Rebased and this should be good to go. |
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 the work on this! Mostly LGTM, just a few nits.
docs-src/getting-started.md
Outdated
- An `acme-lb` GatewayClass which is being managed by a `acme.io/gateway-controller` | ||
controller running in the cluster. |
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 should add a note that users may want to use a GatewayClass provided by the implementation installed in their cluster. If one is not provided, they should ensure that the controller name used here matches a controller name supported by an implementation installed in their cluster.
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 one is not provided
Skipped this part. We shouldn't be encouraging implementers not providing a GatewayClass right now.
traffic to two Kubernetes Services based on HTTP request metadata. | ||
|
||
``` | ||
{% include 'basic-http.yaml' %} |
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 don't have a good solution for this, and this doesn't need to block the PR, but one downside of embedding examples is that the examples could actually begin to skew from the docs. Maybe we should find a way to document which examples are being used in which docs?
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 about this problem too and arrived at the same solution as you.
Essentially adding a link to the guide file as a yaml comment.
Let's continue this in #446.
92e712d
to
8b66933
Compare
@robscott Updated. PTAL. |
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 your work on this! Just a couple small things but otherwise LGTM.
docs-src/getting-started.md
Outdated
- All requests with path beginning with `/some/thing` AND have an HTTP header | ||
`magic: foo` are forwarded to my-service2 Kubernetes Service. | ||
|
||
With this configuration, you now have a Gateway reource which is forwarding |
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.
With this configuration, you now have a Gateway reource which is forwarding | |
With this configuration, you now have a Gateway resource which is forwarding |
## Uninstalling the CRDs | ||
|
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 add a bit of an explanation here, something like:
To uninstall the CRDs and all resources created with them, run the following command. Note that this will remove all GatewayClass and Gateway resources in your cluster. If you have been using these resources for any other purpose, do not uninstall these CRDs.
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.
This is looking great.
Co-authored-by: Rob Scott <[email protected]>
8b66933
to
40beb1b
Compare
@robscott Updated. PTAL. |
Thanks! /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hbagdi, 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 |
Note
This PR builds on top of #425. Please review the last commit only.