Update Readme adding placeholders for AWS and other install options #41

tiina303 · 2021-07-08T19:29:05Z

The readme currently was focused on GCP, tried to make it cover more ground + various nits. Will try to do the actual deployments on GCP and AWS too & document further (and add a bit more details for DO setup where I have manually deployed to already).

macobo · 2021-07-09T08:17:49Z

What should I as a reviewer review here?

I'm not sure what problem this is solving, who the new intended audience is and hence why the structure is like this. Is there a ticket this belongs under?

Proposal: If you're taking over #36 then:

Assign it to yourself - since we're distributed keeping what we're working on clear is really really important.
write up the instructions wholesale, e.g. under the issue
We'll consolidate them into the README either together or I can own it :)

macobo · 2021-07-09T08:18:21Z

charts/posthog/values.yaml

@@ -14,7 +14,7 @@ image:
  pullPolicy: IfNotPresent

 # -- Cloud service being deployed on. Either `gcp` or `aws` or `do` for DigitalOcean
-cloud: "gcp"
+cloud:


This is not a README change.

macobo · 2021-07-09T08:18:34Z

charts/posthog/templates/ingress.yaml

@@ -17,16 +17,6 @@ metadata:
    kubernetes.io/ingress.global-static-ip-name: {{ .Values.ingress.gcp.ip_name | quote }}
    {{- end }}
   {{- end }}
-   {{- if (eq (include "ingress.type" .) "alb") }}


Why remove this as part of README changes?

being lazy ... because I was looking into this as I was writing up the minimum values files, but I can make it a separate PR.

macobo · 2021-07-09T08:19:21Z

charts/posthog/README.md

 ### How can I connect to my ClickHouse instance?

- Get the IP via `kubectl get svc`
+- Get the IP via `kubectl get svc --namespace posthog`


This change is wrong: Other commands here don't encourage specifying the namespace. Users can also specify any namespace they'd like - --namespace posthog varies from one to the other.

charts/posthog/README.md

macobo · 2021-07-09T08:20:24Z

charts/posthog/README.md

 # Note that this is experimental, please let us know how this worked for you.

-cloud: gcp
-ingress:
-  hostname: "posthog.yourcloud.net"


Why? Without this users won't be able to access their cluster.

same, this is part of the required config that I moved up to install

macobo · 2021-07-09T08:20:34Z

charts/posthog/README.md

 # Note that this is experimental, please let us know how this worked for you.

-cloud: gcp


Why? Without this this yaml file does not work.

This is gcp specific, I moved that part up to the minimal required configuration within install section, the sizing stuff is otherwise is platform agnostic.

macobo · 2021-07-09T08:21:54Z

charts/posthog/README.md

 ## Upgrading the chart

 ```console
 helm repo update
-helm upgrade -f values.yaml posthog posthog/posthog
+helm upgrade -f values.yaml --timeout 20m --namespace posthog posthog posthog/posthog


Re upgrade timeout: Why is it needed? Timeout is needed during initial install since clickhouse-operator takes a while to setup. This is not the case with upgrades.

Can't slow migrations take longer than 5min sometimes (what about clickhouse version upgrades)?

macobo · 2021-07-09T08:22:29Z

charts/posthog/README.md

 ```

 Create a `CNAME` record from your desired hostname to the external IP.

+### Setting up email


Structure-wise this is awkwardly placed. This should be under the configuration section as everything else.

Sure this is kind of optional, so happy to move.

macobo · 2021-07-09T08:24:30Z

charts/posthog/README.md


 ```console
 helm repo add posthog https://posthog.github.io/charts-clickhouse/
 helm repo update
-helm install -f values.yaml --timeout 20m posthog posthog/posthog
+helm install -f values.yaml --timeout 20m --create-namespace --namespace posthog posthog posthog/posthog


Why install in a namespace by default in the instructions here.

I think we can assume the users who want separate namespaces will know how to do so. Encouraging to do so by default:

increases command complexity, the amount of flags you need to specify when installing

It makes debugging anything on the cluster more complicated.

If you're installing on a shared cluster, sure that makes sense. However I don't think we should encourage it here.

I think if we always use a namespace it works the same way for everyone being on a shared cluster or not. It's also much easier for us to say just nuke the whole namespace with everything in it without needing to verify they are only running Posthog there first. There's an easy command for kubectl to be in a specific namespace, so you don't need to provide this argument every time. That said I don't feel strongly about this, just figured it could make our lives easier in the long run.

macobo · 2021-07-09T08:25:35Z

charts/posthog/README.md

+helm install -f values.yaml --timeout 20m --create-namespace --namespace posthog posthog posthog/posthog
+```
+
+Where the `values.yaml` has the following minimum content:


This structure is kind of bad and needs to evolve as we support more deployment options.

We now have install section which gives some overview of configuration and then a separate configuration section. As a reader if I have a question I can't follow the structure to find what I'm looking for anymore.

I'd recommend keeping all of configuration in one place and linking to relevant sections instead. As-is this just makes the flow harder to follow.

It'd be great to prepare the AWS instructions first though as once we have them a clearer structure might fall into place in a clearer way.

The way I saw it is "Configuration" is optional stuff, this in here is mandatory (so the user needing to go down to the configuration section added complexity for most folks, who just wanted to make the simplest thing run which I believe is the majority). I'd love if we didn't need it, but I haven't seen a way we could make the chart work with default values for multiple deployments. I looked into it and then only needing 1 value (cloud being the only required setting through values.yaml or --set cloud=gcp or --set cloud=aws), but being able to enable/disable nginx,certManager was tricky based on other values in the yaml sadly.

tiina303 · 2021-07-13T21:01:14Z

Still wip, but going to separate out per platform like Karl suggested.
We could consider changing the guide to start without having a hostname specified as folks might run into issues and things are faster if they don't need to figure out how to access it & wait for DNS to update etc. Then step 2 is securing it.

macobo reviewed Jul 9, 2021

View reviewed changes

charts/posthog/README.md Outdated Show resolved Hide resolved

macobo reviewed Jul 9, 2021

View reviewed changes

charts/posthog/README.md Show resolved Hide resolved

macobo reviewed Jul 9, 2021

View reviewed changes

Rebase

26f14a7

tiina303 force-pushed the readme-placeholders branch from 56c4e89 to 26f14a7 Compare July 13, 2021 20:58

tiina303 added 3 commits July 15, 2021 18:59

Make minimum file work regardless of nginx.enabled default value

0743fdc

fix rebase fail

fefc3ed

nits

296574a

tiina303 merged commit 684c3d6 into main Jul 15, 2021

tiina303 deleted the readme-placeholders branch July 15, 2021 17:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Readme adding placeholders for AWS and other install options #41

Update Readme adding placeholders for AWS and other install options #41

tiina303 commented Jul 8, 2021 •

edited

Loading

macobo commented Jul 9, 2021 •

edited

Loading

macobo Jul 9, 2021

macobo Jul 9, 2021

tiina303 Jul 9, 2021

macobo Jul 9, 2021

macobo Jul 9, 2021

tiina303 Jul 9, 2021

macobo Jul 9, 2021

tiina303 Jul 9, 2021

macobo Jul 9, 2021 •

edited

Loading

tiina303 Jul 9, 2021

macobo Jul 9, 2021

tiina303 Jul 9, 2021

macobo Jul 9, 2021

tiina303 Jul 9, 2021

macobo Jul 9, 2021 •

edited

Loading

tiina303 Jul 9, 2021

tiina303 commented Jul 13, 2021

		# Note that this is experimental, please let us know how this worked for you.

		cloud: gcp

Update Readme adding placeholders for AWS and other install options #41

Update Readme adding placeholders for AWS and other install options #41

Conversation

tiina303 commented Jul 8, 2021 • edited Loading

macobo commented Jul 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macobo Jul 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macobo Jul 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiina303 commented Jul 13, 2021

tiina303 commented Jul 8, 2021 •

edited

Loading

macobo commented Jul 9, 2021 •

edited

Loading

macobo Jul 9, 2021 •

edited

Loading

macobo Jul 9, 2021 •

edited

Loading