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

K8s: install helm chart #957

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from
Draft

K8s: install helm chart #957

wants to merge 12 commits into from

Conversation

kaitlynmichael
Copy link
Contributor

DOC-3433
This content is for the Wisconsin release

Copy link
Contributor

github-actions bot commented Dec 5, 2024

content/operate/kubernetes/deployment/helm.md Outdated Show resolved Hide resolved
content/operate/kubernetes/deployment/helm.md Outdated Show resolved Hide resolved
content/operate/kubernetes/deployment/helm.md Outdated Show resolved Hide resolved
content/operate/kubernetes/deployment/helm.md Outdated Show resolved Hide resolved
content/operate/kubernetes/deployment/helm.md Outdated Show resolved Hide resolved
1. Install the Helm chart from your local directory.

```sh
helm install <release-name> <path-to-chart> \
Copy link

Choose a reason for hiding this comment

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

or
need to be consistent.

I think is better

content/operate/kubernetes/deployment/helm.md Outdated Show resolved Hide resolved
content/operate/kubernetes/deployment/helm.md Outdated Show resolved Hide resolved
content/operate/kubernetes/deployment/helm.md Outdated Show resolved Hide resolved
Comment on lines +36 to +38
-- version <release-name> \
-- namespace <namespace-name> \
-- create-namespace
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to remove the space between the -- and the flag name.

Copy link
Contributor

Choose a reason for hiding this comment

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

This applies to all command examples throughout the document.

1. Install the Helm chart into a new namespace.

```sh
helm install <operator-name> redis/redis-enterprise-operator \
Copy link
Contributor

@zcahana zcahana Dec 12, 2024

Choose a reason for hiding this comment

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

The first placeholder of this command should actually be <release-name> and not <operator-name>.
Confusingly, a "release" in Helm terminology is the user-defined name given a specific installation of the chart (e.g., you can install the same chart multiple times using different "release names").

Maybe we should also explain this briefly, or provide an example command that will convey the idea that could be anything the user wishes.


```sh
helm install <operator-name> redis/redis-enterprise-operator \
-- version <release-name> \
Copy link
Contributor

@zcahana zcahana Dec 12, 2024

Choose a reason for hiding this comment

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

This placeholder should actually be <chart-version> and not <release-name>; Something like 7.8.2-2.

1. Install the Helm chart into a new namespace.

```sh
helm install <operator-name> redis/redis-enterprise-operator \
Copy link
Contributor

@zcahana zcahana Dec 12, 2024

Choose a reason for hiding this comment

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

The redis/redis-enterprise-operator parameter is made up of <repo-name>/<chart-name>.
The is the same one we use in step (1) above (helm repo add <repo-name>). We should get it aligned to either redis or redis-enterprise.

Comment on lines +26 to +30
1. Add the `redis` repository.

```sh
helm repo add redis-enterprise https://helm.redis.io/
```
Copy link
Contributor

@zcahana zcahana Dec 12, 2024

Choose a reason for hiding this comment

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

Here too, we should use a single repo name in both the sentence "Add the <repo-name> repository" and the command example.


### Install from local directory

1. Find the latest release on the [redis-enterprise-k8s-docs Github](https://github.com/RedisLabs/redis-enterprise-k8s-docs/releases) and download the `tar.gz` source code into a local directory.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can also consider this, I think it looks a little nicer when rendered:

Suggested change
1. Find the latest release on the [redis-enterprise-k8s-docs Github](https://github.com/RedisLabs/redis-enterprise-k8s-docs/releases) and download the `tar.gz` source code into a local directory.
1. Find the latest release on the [redis-enterprise-k8s-docs](https://github.com/RedisLabs/redis-enterprise-k8s-docs/releases) repo and download the `tar.gz` source code into a local directory.


### Specify values during install

1. View configurable values with `helm show values redis/redis --version <release-name>`.
Copy link
Contributor

@zcahana zcahana Dec 12, 2024

Choose a reason for hiding this comment

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

The argument to helm show values is also a <repo-name>/<chart-name>.
The <chart-name> should be redis-enterprise-operator, and the <repo-name> should best align with whatever you pick in the install section above (either redis or redis-enterprise).


### Specify values during install

1. View configurable values with `helm show values redis/redis --version <release-name>`.
Copy link
Contributor

@zcahana zcahana Dec 12, 2024

Choose a reason for hiding this comment

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

Also - same comment about <chart-version> instead of <release-name>.

--set <key2>=<value2>
```

### Install with values file
Copy link
Contributor

Choose a reason for hiding this comment

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

This section is largely repeating the previous one, maybe we should just mention that the values may be overridden either by the --set flag or via --values file?

Comment on lines +80 to +81
1. Create a YAML file containing the configuration values you want to set.
1. Create a YAML file to specify the values you want to configure.
Copy link
Contributor

Choose a reason for hiding this comment

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

Only one of these should win.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants