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

Reorg helm chart docs #6849

Merged
merged 1 commit into from
Dec 4, 2019
Merged

Reorg helm chart docs #6849

merged 1 commit into from
Dec 4, 2019

Conversation

lkysow
Copy link
Member

@lkysow lkysow commented Nov 28, 2019

  • Remove duplicate install instructions from the Helm Chart page and
    kept them in Running Consul
  • Renamed Helm Chart to Helm Chart Reference because that's mostly what
    it contains (along with some examples)
  • Renamed Running Consul to Installing Consul
  • Changed instructions to be for installing using Helm 3 and added
    notes if using Helm 2
  • Used release name "hashicorp" so subsequent instructions can be more
    concise and pastable, e.g. "port forward to svc/hashicorp-consul-server" vs. "port
    forward to svc/-consul-server"
  • Use config.yaml as the name for the override values file since it
    differentiates from the default values.yaml file and its the name of the
    file used in the helm docs
    (https://helm.sh/docs/intro/using_helm/#customizing-the-chart-before-installing)

@lkysow lkysow requested a review from a team November 28, 2019 21:29
@@ -63,17 +65,7 @@ There are several ways to try Consul with Kubernetes in different environments.
- The [Consul and Kubernetes Deployment](
https://learn.hashicorp.com/consul/day-1-operations/kubernetes-deployment-guide?utm_source=consul.io&utm_medium=docs) guide covers the necessary steps to install and configure a new Consul cluster on Kubernetes in production.

## "consul-k8s" Project
Copy link
Member Author

Choose a reason for hiding this comment

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

Since this documentation doesn't seem important to users I thought it best to remove it. Especially from the index page.

Copy link
Contributor

Choose a reason for hiding this comment

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

How do we know it's not important to users?

Copy link
Member Author

Choose a reason for hiding this comment

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

My thinking is that most users will install the helm chart and use the values.yaml to configure their installation. So when they first come to this page, they don't need to know about the consul-k8s repo.

@@ -1,12 +1,12 @@
---
layout: "docs"
page_title: "Running Consul - Kubernetes"
page_title: "Installing Consul on Kubernetes - Kubernetes"
Copy link
Member Author

Choose a reason for hiding this comment

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

I felt Installing was better than Running since that's what I'd click on looking for installation instructions. We use Installing Consul in the top level docs as well.


# Checkout a tagged version
$ git checkout v0.1.0
```bash
Copy link
Member Author

Choose a reason for hiding this comment

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

I moved these instructions over from the helm.html page. I think they're a bit cleaner

`helm upgrade`. See the [Upgrading Consul on Kubernetes](/docs/platform/k8s/run.html#upgrading-consul-on-kubernetes)
section for more details.

## Uninstalling Consul
Copy link
Member Author

Choose a reason for hiding this comment

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

I've added these instructions here.

Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

Copy link
Contributor

@ishustava ishustava 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 doing the work of reviewing and reorganizing the docs 🥇 💯 I left some comments. Overall it looks good to me. Other than small improvements, the one important thing to me is to keep the security warning that got removed.

@@ -63,17 +65,7 @@ There are several ways to try Consul with Kubernetes in different environments.
- The [Consul and Kubernetes Deployment](
https://learn.hashicorp.com/consul/day-1-operations/kubernetes-deployment-guide?utm_source=consul.io&utm_medium=docs) guide covers the necessary steps to install and configure a new Consul cluster on Kubernetes in production.

## "consul-k8s" Project
Copy link
Contributor

Choose a reason for hiding this comment

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

How do we know it's not important to users?

@@ -17,14 +17,17 @@ This page starts with a large how-to section for various specific tasks.
To learn more about the general architecture of Consul on Kubernetes, scroll
down to the [architecture](/docs/platform/k8s/run.html#architecture) section. If you would like to get hands-on experience testing Consul on Kubernetes, try the step-by-step beginner tutorial with an accompanying video in the [Minikube with Consul guide](https://learn.hashicorp.com/consul/getting-started-k8s/minikube?utm_source=consul.io&utm_medium=docs)

## Helm Chart
## Helm Chart Installation

The recommended way to run Consul on Kubernetes is via the
[Helm chart](/docs/platform/k8s/helm.html). This will install and configure
all the necessary components to run Consul. The configuration enables you
to run just a server cluster, just a client cluster, or both. Using the Helm
chart, you can have a full Consul deployment up and running in seconds.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not related to your change, but since I'm reading it, should this say "in minutes"? I feel like installing it in seconds is a bit of stretch.

Copy link
Member Author

Choose a reason for hiding this comment

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

Seconds >60? 😆 I'll change it to a couple minutes

# Clone the chart repo
$ git clone https://github.com/hashicorp/consul-helm.git
$ cd consul-helm
Clone the chart at that version, for example if the latest version is
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Clone the chart at that version, for example if the latest version is
Clone the chart at that version, for example, if the latest version is

Comment on lines +88 to 91
```sh
$ helm install hashicorp ./consul-helm
NAME: hashicorp
...
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be nice to say here explicitly that these instructions are for Helm 3. I feel like Helm 3 hasn't become the de facto standard yet and people are still transitioning to it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

`helm upgrade`. See the [Upgrading Consul on Kubernetes](/docs/platform/k8s/run.html#upgrading-consul-on-kubernetes)
section for more details.

## Uninstalling Consul
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!


-> If using Helm 2, run `helm delete --purge hashicorp`

After deleting the Helm release, you need to delete the PersistentVolumeClaims
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
After deleting the Helm release, you need to delete the PersistentVolumeClaims
After deleting the Helm release, you need to delete the `PersistentVolumeClaims`

-> If using Helm 2, run `helm delete --purge hashicorp`

After deleting the Helm release, you need to delete the PersistentVolumeClaims
that store Consul's data. These are not deleted by Helm due to a [bug](https://github.com/helm/helm/issues/5156).
Copy link
Contributor

Choose a reason for hiding this comment

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

Kind of nitpicking here, but technically pvcs don't store data.

Suggested change
that store Consul's data. These are not deleted by Helm due to a [bug](https://github.com/helm/helm/issues/5156).
for persistent volumes that store Consul's data. These are not deleted by Helm due to a [bug](https://github.com/helm/helm/issues/5156).

For security reasons, it isn't exposed via a Service by default so you must
use `kubectl port-forward` to visit the UI. Once the port is forwarded as
shown below, navigate your browser to `http://localhost:8500`.
For security reasons, it isn't exposed via a LoadBalancer Service by default so you must
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
For security reasons, it isn't exposed via a LoadBalancer Service by default so you must
For security reasons, it isn't exposed via a `LoadBalancer` Service by default so you must

Comment on lines 90 to 91
The UI can also be exposed via a Kubernetes Service. To do this, configure
the [`ui.service` chart values](/docs/platform/k8s/helm.html#v-ui-service).
Copy link
Contributor

@ishustava ishustava Dec 3, 2019

Choose a reason for hiding this comment

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

I think this comment is still valuable if we change the wording to say LoadBalancer service rather than "Kubernetes Service". Did you think it's redundant?

Copy link
Member Author

Choose a reason for hiding this comment

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

If they're not using ACLs then they're exposing the Consul HTTP API to the world and even with ACLs the anonymous token lets you read all nodes and services. So I thought it best not to encourage it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not necessarily, this depends on the security groups and whether they use an internal or an external load balancer. Currently, we allow setting annotations for the UI service, and on AWS, for example, you can create an internal load balancer with an annotation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay I'll add it back.

website/source/docs/platform/k8s/helm.html.md Show resolved Hide resolved
@lkysow lkysow force-pushed the helm-chart-docs branch 3 times, most recently from 47b3b7f to ad85663 Compare December 3, 2019 19:34
@codecov
Copy link

codecov bot commented Dec 3, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@eda868a). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #6849   +/-   ##
=========================================
  Coverage          ?   65.79%           
=========================================
  Files             ?      435           
  Lines             ?    52398           
  Branches          ?        0           
=========================================
  Hits              ?    34473           
  Misses            ?    13792           
  Partials          ?     4133

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eda868a...5412ba9. Read the comment docs.

Copy link
Contributor

@ishustava ishustava left a comment

Choose a reason for hiding this comment

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

👍 😀

- Remove duplicate install instructions from the Helm Chart page and
kept them in Running Consul
- Renamed Helm Chart to Helm Chart Reference because that's mostly what
it contains (along with some examples)
- Renamed Running Consul to Installing Consul
- Changed instructions to be for installing using Helm 3 and added
  notes if using Helm 2
- Used release name "hashicorp" so subsequent instructions can be more
concise and pastable, e.g. "port forward to svc/hashicorp-consul-server" vs. "port
forward to svc/<your release name>-consul-server"
- Use config.yaml as the name for the override values file since it
differentiates from the default values.yaml file and its the name of the
file used in the helm docs
(https://helm.sh/docs/intro/using_helm/#customizing-the-chart-before-installing)
@lkysow lkysow merged commit 7ac6a08 into master Dec 4, 2019
@lkysow lkysow deleted the helm-chart-docs branch December 4, 2019 01:49
@ghost
Copy link

ghost commented Jan 25, 2020

Hey there,

This issue has been automatically locked because it is closed and there hasn't been any activity for at least 30 days.

If you are still experiencing problems, or still have questions, feel free to open a new one 👍.

@ghost ghost locked and limited conversation to collaborators Jan 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants