-
Notifications
You must be signed in to change notification settings - Fork 41
docs: minor improvements #217
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
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.
after some nitpicking, LGTM
@@ -18,7 +18,7 @@ The Operator to install and manage the lifecycle of the [Kuadrant](https://githu | |||
* [If you are an <em>API Provider</em>](#if-you-are-an-api-provider) | |||
* [If you are a <em>Cluster Operator</em>](#if-you-are-a-cluster-operator) | |||
* [User guides](#user-guides) | |||
* [<a href="/doc/rate-limiting.md">Kuadrant Rate Limiting</a>](#kuadrant-rate-limiting) | |||
* [<a href="doc/rate-limiting.md">Kuadrant Rate Limiting</a>](#kuadrant-rate-limiting) |
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.
Just for curiosity, is /doc/rate-limiting.md
wrong? the link seems to be working (on Linux/Firefox)
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 leading slash breaks it in https://docs.kuadrant.io/kuadrant-operator/. Not sure if using the simple relative path will solve it tho.
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.
It works but it is broken in the website for the above in the table of contents ^^ . I am also not sure will this change fix it since it links to https://docs.kuadrant.io/doc/rate-limiting.md vs https://docs.kuadrant.io/kuadrant-operator/doc/rate-limiting/ which seems to be the correct url instead 🤷
Though this change is also to give consitency to references to other docs in the repo. In most other links, we use docs/x
instead of /docs/x
🤔
@@ -125,12 +126,13 @@ kubectl create namespace kuadrant | |||
|
|||
Apply the `Kuadrant` custom resource: | |||
|
|||
```yaml | |||
kubectl apply -n kuadrant -f -<<EOF | |||
```sh |
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.
nitpick: technically it is a shell script, but I would be highlighting the yaml 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.
Plus, if you keep it as sh
, you can run it in vscode with https://marketplace.visualstudio.com/items?itemName=guicassolato.tothom 😜
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.
Yeah, this is a hard one 🤔 TBH previously, we are not consistent in this use for code blocks. In some places, we use yaml
vs sh
.
While highlighting for yaml is nice, I've went with sh
since it is a shell script and as Gui hinted for vscode, in Goland IDE there is a similar function to run commands directly from markdowns for shell commands also, which is also a nice feature
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.
take my money
@@ -49,12 +41,6 @@ Create the `HTTPRoute`: | |||
kubectl apply -f examples/toystore/httproute.yaml | |||
``` | |||
|
|||
Expose the API: |
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 needed when not running in Linux
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.
On macOS at least, it's not needed anymore.
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've added the following note a bit further down the guide instead:
It should return `200 OK`.
**Note**: This only works out of the box on linux environments. If not on linux,
you may need to forward ports
kubectl port-forward -n istio-system service/istio-ingressgateway 9080:80 &
This keeps it consistent with the other user guides which specifies this note instead of explicity requiring the user to port forward as a guide step 🤔
I'll push a commit for fixing #216 as part of this PR |
… instead of 200 Closes: Kuadrant#216
* docs: minor improvements * user-guide: fix authorization using k8s service account returning 403 instead of 200 Closes: #216
General improvements in docs:
Kuadrant
instance