-
Notifications
You must be signed in to change notification settings - Fork 979
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
Docs: Update Getting started and docs clean-up #1116
Conversation
✔️ Deploy Preview for karpenter-docs-prod ready! 🔨 Explore the source changes: f13189e 🔍 Inspect the deploy log: https://app.netlify.com/sites/karpenter-docs-prod/deploys/61eaf2507321800007334784 😎 Browse the preview: https://deploy-preview-1116--karpenter-docs-prod.netlify.app |
thanks and welcome! :) we have previously declined to include fargate in the getting started guide. reasons include "mixed messaging" on node mgmt and introduction of an aws proprietary concept into cluster operations. Can you (A) remove fargate from the getting started guide, (B) add a comment explaining why it's important to include, or (C) move the fargate content to a different area (e.g., new task)? Thanks! Geoffrey |
website/content/en/docs/getting-started-with-terraform/_index.md
Outdated
Show resolved
Hide resolved
Hi @geoffcline! Thanks for your feedback. I believe having Fargate as an deployment howto is a valid option since it currently only works on AWS anyways and using Fargate for those controllers seems a really good use case :) I'm happy to either break it to a separate section or add additional comments to the docs. Please let me know what would be your preference. |
One of the things we're worried about is docs sprawl. Given the amount of resources we have to keep the docs accurate and understandable, we need to be careful about how much content is in scope. At the end of the day, there are so many ways to configure kubernetes on aws (fargate,mng,kops) as well as ways to configure addons (irsa/k2iam, cnis, etc), that we can't hope to get them all. Instead, the goal is to document ~1 happy path that demonstrates the components involved, and customers can swap in alternatives as makes sense for their use case. I'd love to see this kind of content in a medium blog, which we will happily link to on the main README. |
That makes sense and that is intent as well. I think amount of change here is minimal and it's just adding Fargate option (1 profile with two namespaces) to Terraform path. Maintenance burden is quite low as this bit is not going to change. If you would still prefer to not include it in getting started, I'm happy to link it to my repo. |
Hey @mbevc1, I think we're going to punt on this scope for now. I'd be happy to link to from our main README to "unofficial" docs for Karpenter+Fargate. |
b82033c
to
bd30e9a
Compare
@geoffcline @ellistarn thanks for the feedback. Removed Fargate scenario here and only leaving minor docs updates. |
343f557
to
3931c8e
Compare
@@ -311,10 +306,6 @@ spec: | |||
cpu: 1000 | |||
provider: | |||
instanceProfile: KarpenterNodeInstanceProfile-${CLUSTER_NAME} | |||
subnetSelector: |
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.
These are now required, as of v0.5.5
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, let me put it back now, miseed it from 0.5.5 release notes :)
Thanks Marko, just one minor detail left. |
website/content/en/preview/getting-started-with-terraform/_index.md
Outdated
Show resolved
Hide resolved
website/content/en/v0.5.5/getting-started-with-terraform/_index.md
Outdated
Show resolved
Hide resolved
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.
Thanks!
👋 @geoffcline all good on your end for merging this one? |
I added a reminder to review this Monday morning :) thanks for the ping |
approved and merged! in the future you only need to update /preview, no need to update the archive versions. |
Thanks 👍 |
1. Issue, if available:
n/a
2. Description of changes:
3. How was this change tested?
n/a
4. Does this change impact docs?
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.