Skip to content

docs: Add Helm guides#6390

Merged
webvictim merged 19 commits intomasterfrom
gus/helm/docs
May 17, 2021
Merged

docs: Add Helm guides#6390
webvictim merged 19 commits intomasterfrom
gus/helm/docs

Conversation

@webvictim
Copy link
Copy Markdown
Contributor

Updates #5582

Still a work in progress.

@webvictim
Copy link
Copy Markdown
Contributor Author

webvictim commented Apr 13, 2021

Copy link
Copy Markdown
Contributor

@klizhentas klizhentas left a comment

Choose a reason for hiding this comment

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

I've done the first pass. Monumental work. General notes:

  • Let's move all the content to Kubernetes access
  • Let's split getting started, architecture and reference to separate docs

Ping me if you have any questions, would be happy to discuss over quick chat.

Comment thread docs/pages/helm/guides/aws.mdx Outdated
Comment thread docs/pages/helm/guides/gcp.mdx Outdated
Comment thread docs/pages/helm/guides/gcp.mdx Outdated
Comment thread docs/pages/helm/guides/gcp.mdx Outdated
Comment thread docs/pages/helm/guides/gcp.mdx Outdated
Comment thread docs/pages/helm/guides/ownconfig.mdx Outdated
Comment thread docs/pages/helm/guides/ownconfig.mdx Outdated
Comment thread docs/pages/helm/guides/ownconfig.mdx Outdated
Comment thread docs/pages/helm/guides/ownconfig.mdx Outdated
Comment thread docs/pages/helm/guides/standalone.mdx Outdated
@webvictim webvictim force-pushed the gus/helm/docs branch 2 times, most recently from 93a89a4 to 9d711a0 Compare April 14, 2021 21:00
@webvictim
Copy link
Copy Markdown
Contributor Author

@klizhentas I've addressed pretty much all your feedback. Just need to write the migration guide and add some details on cert-manager for AWS and GKE.

Vercel: https://teleport-docs-next-git-gus-helmdocs-gravitational.vercel.app/docs/ver/7.0/kubernetes-access/helm/guides/

@webvictim webvictim force-pushed the gus/helm/docs branch 2 times, most recently from 0660388 to 3ad7f87 Compare April 26, 2021 17:17
@webvictim
Copy link
Copy Markdown
Contributor Author

cert-manager details have been added. Only thing left is the migration guide, which I'll work on this week.

@webvictim webvictim marked this pull request as ready for review April 26, 2021 17:28
@webvictim webvictim requested a review from klizhentas April 26, 2021 17:28
@webvictim
Copy link
Copy Markdown
Contributor Author

@klizhentas Please re-review.

@webvictim
Copy link
Copy Markdown
Contributor Author

@webvictim
Copy link
Copy Markdown
Contributor Author

@klizhentas Ping

@klizhentas
Copy link
Copy Markdown
Contributor

@webvictim will review tomorrow, thanks Gus!

Copy link
Copy Markdown
Contributor

@klizhentas klizhentas left a comment

Choose a reason for hiding this comment

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

Overall, solid work, great attention to detail and props for taking care of cert-manager, migration and reference. High quality work, @webvictim well done.

I have a ton of nitpicks, mostly styling and using new features of the docs. Once applied, should be good to go.

Comment thread docs/config.json Outdated
Comment thread docs/pages/kubernetes-access/helm/guides.mdx Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This does not render as a header, so hard to separate sections from each other. Please try this

## Chart `teleport-cluster`

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is annoying - it used to render as a header but it stopped when I rebased so clearly this was changed at some point.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This has the same level as the helm chart, but is a property of the chart. Let's try this instead and see how it renders

### Parameter `clusterName`

This will also make TOC manageable., right now it overflows.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

A multi-level TOC which uses the level of the header comments would be really nice, or some other way to auto-generate a TOC at the top of the file rather than using the side nav.

Comment thread docs/pages/kubernetes-access/helm/reference.mdx Outdated
Comment thread docs/pages/kubernetes-access/helm/guides/custom.mdx Outdated
Comment thread docs/pages/kubernetes-access/helm/guides/custom.mdx Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: sometimes we use $ and sometimes not, probably remove elsewhere too?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah you're right. My intention is to use $ whenever there is example output after the command (to separate the two) but this one was inconsistent.

Comment thread docs/pages/kubernetes-access/helm/guides/custom.mdx Outdated
Comment thread docs/pages/kubernetes-access/helm/guides/migration.mdx Outdated
@webvictim
Copy link
Copy Markdown
Contributor Author

webvictim commented May 10, 2021

@klizhentas This has addressed the majority of your nits. I'm wondering if there's a better way we can handle the TOC on the reference pages rather than having to manually generate one.

Also, the formatting of fixed-width headers seems to have changed between when I started this PR and now - they used to scale to the size of header used (i.e. when using ## you'd get bigger text) but now for some reason they're all rendered at a fixed size which makes everything look weird. I've raised an issue in next for this: https://github.com/gravitational/next/issues/231

@klizhentas
Copy link
Copy Markdown
Contributor

@webvictim thank you, will review today.

Copy link
Copy Markdown
Contributor

@klizhentas klizhentas left a comment

Choose a reason for hiding this comment

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

LGTM. :shipit: 🚢 🚀

@webvictim
Copy link
Copy Markdown
Contributor Author

Thanks @klizhentas - could you take a look at #6344 too please? Andrew’s already reviewed the code so mostly needs a code owner stamp.

@webvictim webvictim requested a review from inertial-frame as a code owner May 14, 2021 18:46
@webvictim webvictim merged commit 906bfeb into master May 17, 2021
@webvictim webvictim deleted the gus/helm/docs branch May 17, 2021 13:38
webvictim added a commit that referenced this pull request May 17, 2021
webvictim added a commit that referenced this pull request May 20, 2021
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.

2 participants