Skip to content
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

Merged
merged 1 commit into from
Nov 5, 2020

Conversation

hbagdi
Copy link
Contributor

@hbagdi hbagdi commented Oct 27, 2020

Note

This PR builds on top of #425. Please review the last commit only.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 27, 2020
Copy link
Member

@robscott robscott left a 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!

Comment on lines +11 to +12
kubectl kustomize "github.com/kubernetes-sigs/service-apis/config/crd?ref=master" \
| kubectl apply -f -
Copy link
Member

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).

Copy link
Contributor Author

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.


## Install an implementation

Service APIs is a set of specification that multiple implementations confirm to.
Copy link
Member

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."


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.
Copy link
Member

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.


- 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
Copy link
Member

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.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above

Comment on lines 5 to 6
Service APIs is a collection of Kubernetes Custom Resource Definitions that need
to be installed into any Kubernetes cluster.
Copy link
Member

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."

@hbagdi
Copy link
Contributor Author

hbagdi commented Oct 28, 2020

Depends on #425
/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 28, 2020
@hbagdi hbagdi force-pushed the getting-started-guide branch 2 times, most recently from 8502d40 to f6a78f3 Compare October 28, 2020 16:09
@hbagdi
Copy link
Contributor Author

hbagdi commented Oct 28, 2020

golint went missing so I rebased on master to ensure that I pull in the fixes we put in but not luck.
Is it the CI acting up?

@hbagdi
Copy link
Contributor Author

hbagdi commented Oct 28, 2020

/retest

@robscott
Copy link
Member

@hbagdi Our verify output is super confusing, in this case it looks like another run of make generate is needed:

Uncommitted changes in generated sources:
 M docs/getting-started/index.html

@hbagdi hbagdi force-pushed the getting-started-guide branch 2 times, most recently from 54b393f to a50ca52 Compare October 29, 2020 14:51
@hbagdi
Copy link
Contributor Author

hbagdi commented Oct 29, 2020

Rebased and this should be good to go.
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 29, 2020
Copy link
Member

@robscott robscott left a 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 Show resolved Hide resolved
Comment on lines 28 to 29
- An `acme-lb` GatewayClass which is being managed by a `acme.io/gateway-controller`
controller running in the cluster.
Copy link
Member

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.

Copy link
Contributor Author

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' %}
Copy link
Member

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?

Copy link
Contributor Author

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.

docs-src/getting-started.md Outdated Show resolved Hide resolved
docs-src/getting-started.md Outdated Show resolved Hide resolved
@hbagdi
Copy link
Contributor Author

hbagdi commented Nov 2, 2020

@robscott Updated. PTAL.

@hbagdi hbagdi added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Nov 2, 2020
Copy link
Member

@robscott robscott left a 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.

- 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
With this configuration, you now have a Gateway reource which is forwarding
With this configuration, you now have a Gateway resource which is forwarding

Comment on lines +51 to +52
## Uninstalling the CRDs

Copy link
Member

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.

Copy link
Contributor

@youngnick youngnick left a 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.

@hbagdi
Copy link
Contributor Author

hbagdi commented Nov 5, 2020

@robscott Updated. PTAL.

@robscott
Copy link
Member

robscott commented Nov 5, 2020

Thanks!

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 5, 2020
@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 5, 2020
@k8s-ci-robot k8s-ci-robot merged commit edcbeaa into kubernetes-sigs:master Nov 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants