-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Updating helm docs with additionalVault and ACLs refactor functionality. #12669
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.
@jmurret I left some feedback and comments in the pr. Some of the definitions are confusing and tough to follow. Take a look at ones I called out and let's try to simplify the information.
website/content/docs/k8s/helm.mdx
Outdated
@@ -169,16 +169,35 @@ Use these links to navigate to a particular top-level stanza. | |||
- gossip encryption key defined by `global.gossipEncryption.secretName`. | |||
To discover the service account name of the Consul client, run | |||
```shell-session | |||
$ helm template --show-only templates/client-serviceaccount.yaml <release-name> charts/consul | |||
$ helm template --show-only templates/client-serviceaccount.yaml <release-name> hashicorp/consul |
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.
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.
How are you able to preview this? I would love to also be able to do this myself.
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 added a comment in the PR that explains this. But yeah, it's still breaking 😢
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.
Maybe it needs a newline before and after? or maybe it's wrong indentation?
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.
@karl-cardenas-coding can you elaborate exactly what should happen here in terms of the visual we should see? When I look at this preview, all code blocks match the indentation of the lines above them and this one seems to as well. Am I trying to widen this code block?
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.
oh sorry. I thought the indentation was wrong. I see it now. :face-palm.
the markdown is showing up in the rendered code block.
Thanks @karl-cardenas-coding! I'll take a look and revise. |
Hi @karl-cardenas-coding, thank you for the thoughtful review. I have tried to address all of your commits if you want to look at the latest commit. |
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.
@jmurret Much better and thank you for making the changes. I left you two minor suggestions that need your input. Besides that, the only remaining item are the shell-sessions
blocks that are not rendering correctly.
website/content/docs/k8s/helm.mdx
Outdated
@@ -169,16 +169,35 @@ Use these links to navigate to a particular top-level stanza. | |||
- gossip encryption key defined by `global.gossipEncryption.secretName`. | |||
To discover the service account name of the Consul client, run | |||
```shell-session | |||
$ helm template --show-only templates/client-serviceaccount.yaml <release-name> charts/consul | |||
$ helm template --show-only templates/client-serviceaccount.yaml <release-name> hashicorp/consul |
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 added a comment in the PR that explains this. But yeah, it's still breaking 😢
website/content/docs/k8s/helm.mdx
Outdated
- `adminPartitionsRole` ((#v-global-secretsbackend-vault-adminpartitionsrole)) (`string: ""`) - <EnterpriseAlert inline /> A Vault role to allow Kubernetes job that creates a Consul partition for this Helm chart (`partition-init`) | ||
to read Vault secret for the partition ACL token. |
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.
- `adminPartitionsRole` ((#v-global-secretsbackend-vault-adminpartitionsrole)) (`string: ""`) - <EnterpriseAlert inline /> A Vault role to allow Kubernetes job that creates a Consul partition for this Helm chart (`partition-init`) | |
to read Vault secret for the partition ACL token. | |
- `adminPartitionsRole` ((#v-global-secretsbackend-vault-adminpartitionsrole)) (`string: ""`) - <EnterpriseAlert inline /> A Vault role to allow a Kubernetes job create a Consul partition for this Helm chart deployment (`partition-init`). This role allows Consul to read Vault secret for the respective partition ACL token. |
Is this correct? I'm still a bit lost here 😅
website/content/docs/k8s/helm.mdx
Outdated
@@ -169,16 +169,35 @@ Use these links to navigate to a particular top-level stanza. | |||
- gossip encryption key defined by `global.gossipEncryption.secretName`. | |||
To discover the service account name of the Consul client, run | |||
```shell-session | |||
$ helm template --show-only templates/client-serviceaccount.yaml <release-name> charts/consul | |||
$ helm template --show-only templates/client-serviceaccount.yaml <release-name> hashicorp/consul |
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.
Maybe it needs a newline before and after? or maybe it's wrong indentation?
@karl-cardenas-coding The shell-sessions are fixed. It apparently expects there to only be one space between the |
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.
👍🏼
- Fix indentation. - Fix description of secretName and secretKey to be consistent - Change description of manageACLsRole to be more clear. - Make the added vault role field descriptions consistent
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/623323. |
🍒✅ Cherry pick of commit 2bc11a5 onto |
…ty. (#12669) * Updating helm docs with additionalVault and ACLs refactor funtionality. * PR Feedback corrections. - Fix indentation. - Fix description of secretName and secretKey to be consistent - Change description of manageACLsRole to be more clear. - Make the added vault role field descriptions consistent * PR Feedback - correcting description for adminPartitionsRole * Fixing broken shell sessions * Fixing broken shell sessions by changing shell-session tobecloser tocomment marker
🍒✅ Cherry pick of commit 2bc11a5 onto |
…ty. (#12669) * Updating helm docs with additionalVault and ACLs refactor funtionality. * PR Feedback corrections. - Fix indentation. - Fix description of secretName and secretKey to be consistent - Change description of manageACLsRole to be more clear. - Make the added vault role field descriptions consistent * PR Feedback - correcting description for adminPartitionsRole * Fixing broken shell sessions * Fixing broken shell sessions by changing shell-session tobecloser tocomment marker
No description provided.