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

#168 doc improvements and editing out of date api versions. #276

Merged
merged 3 commits into from
Oct 13, 2021

Conversation

KianTigger
Copy link
Contributor

Added missing elements to validate-tmpl.yaml.

Edited yaml files to get rid of warnings about api versions.

In install/install.md changed numbers after “Assuming your cluster has cert-manager installed:” (not 1-6)

In docker/readme.md first changed command to be ‘docker build -f docker/Dockerfile -t trow ..’

Added note in trow/quick-install.md noting that clusterrolebinding must be done every time

Copy link
Contributor

@amouat amouat left a comment

Choose a reason for hiding this comment

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

Some nice small improvements and fixes here.

I'm not sure about adding the "install" prefix everywhere. I think it would be cleaner to say at the start "this assumes you are in the install directory" or similar (same for the "docker" directory thing).

Try to keep lines in the md files to a certain length (it's probably about 100 columns?).

QUICK-INSTALL.md Show resolved Hide resolved
charts/trow/templates/ingress.yaml Show resolved Hide resolved
docker/README.md Outdated
@@ -4,7 +4,7 @@ The easiest way to build Trow is via Dockerfile. Either run `build.sh` from this
something similar to following:

```
docker build -f Dockerfile -t trow ..
docker build -f docker/Dockerfile -t trow ..
Copy link
Contributor

Choose a reason for hiding this comment

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

So the README file is in the docker directory i.e. you don't need the docker prefix if you run it from the same directory as the README. We should say that though.

2) Run `kubectl apply -k overlays/mycluster` from the install directory.
3) Set the DNS for your domain to point to the IP for your ingress, which you can find with `kubectl
get ingress -n trow`. Note that in GKE, this IP is subject to change unless you obtain a static IP.
4) Run `kubectl apply -k install/overlays/mycluster` from the install directory.
Copy link
Contributor

Choose a reason for hiding this comment

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

this isn't right (if you run it from the install directory, you don't need the install bit)

@@ -0,0 +1,15 @@
apiVersion: networking.gke.io/v1beta1
Copy link
Contributor

Choose a reason for hiding this comment

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

what's this file for? Is it meant to be in the docs directory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the example file you would create if you follow the helm install instructions. Thought it might be useful to just have one created as a template? I can remove it if you think it's not needed.

Copy link
Contributor

@amouat amouat Oct 13, 2021

Choose a reason for hiding this comment

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

I'm going to remove this. There's a bit of confusion here due to poor documentation in the Helm instructions.

quick-install/validate-tmpl.yaml Outdated Show resolved Hide resolved
@amouat amouat merged commit 6e2aed0 into Trow-Registry:main Oct 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants