-
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
Getting Started and Other Docs #527
Conversation
To get ahead of potential rabbit holes, I'm working on the hosting side of things now and will surface a PR with a recommendation |
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.
Nice work! Just a couple of nits and possible suggestions.
docs/404.html
Outdated
@@ -3,36 +3,36 @@ | |||
<head> | |||
<meta charset="utf-8"> | |||
<meta name="viewport" content="width=device-width, initial-scale=1, shrink-to-fit=no"> | |||
<meta name="generator" content="Hugo 0.85.0" /><META NAME="ROBOTS" CONTENT="NOINDEX, NOFOLLOW"> | |||
<meta name="generator" content="Hugo 0.84.1" /><META NAME="ROBOTS" CONTENT="NOINDEX, NOFOLLOW"> |
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.
Is this referring to a version of Hugo? Is there a reason we wouldn't want to use the most recent version (seems like 0.86.0
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.
this is mooted by #528 build on netlify
Use helm to deploy Karpenter to the cluster. | ||
|
||
We created a Kubernetes service account when we created the cluster using | ||
eksctl. Thus, we don't need the helm chart to do that. |
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.
Great. WDYT about having a command here (or a link to a command) to patch the serviceAccount for BYOC experiences or debugging purposes?
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 agree but i think that's out of scope for the PR right now.
|
||
### Create Workload | ||
|
||
This deployment uses the [pause image](https://www.ianlewis.org/en/almighty-pause-container) and starts with zero replicas. |
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.
We also have the feature to include a karpenter.sh/do-not-evict:true
annotation for pods they don't want to evict. This is a pretty specific feature, so it may overcomplicate this guide. WDYT?
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'll include that in the annotations section
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.
Great work! I think this is definitely on the right track and a huge improvement over the current in-repo docs. Let's keep pushing on these to get them completed and hopefully we can get this published before the Bug Bash tomorrow.
website/content/en/docs/faq.md
Outdated
Yes. The Kubernetes Eviction API will not delete pods that violate a [Pod Disruption Budget (PDB)](https://kubernetes.io/docs/tasks/run-application/configure-pdb/). It also disallows eviction of any pod covered by multiple PDBs, so most users will want to avoid overlapping selectors. See [this](https://kubernetes.io/docs/concepts/workloads/pods/disruptions/#pod-disruption-budgets) for more. | ||
### Does Karpenter support scale to zero? | ||
Yes. Provisioners start at zero and launch or terminate nodes as necessary. We recommend that customers maintain a small amount of static capacity to bootstrap system controllers or run Karpenter outside of their cluster. | ||
## Compatibility |
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.
Let's bump this section up since these are more common questions than some of the more prominently placed ones.
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.
moved ### Does Karpenter support scale to zero?
to under ## General
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 was referring to the ## Compatibility
section, not scale to zero
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.
ah ty
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.
moved compatibility to the section after general.
website/public/404.html
Outdated
@@ -0,0 +1,128 @@ | |||
<!doctype html> |
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 think we should consider removing the build generated files under public/
as well as those built under ../docs/
since they cause a ton of noise in these PRs. Thoughts?
Even if we decide to keep these built files in the PR, there's no reason we'd need to have both these in public/
and those in ../docs/
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 have removed website/public
we will remove ../docs/
in the netlify pr (or a follow up)
Co-authored-by: Alex Kestner <[email protected]>
2c2ee65
to
ef45aa4
Compare
Hey @geoffcline, it looks like you've force pushed to this branch a few times since it was reviewed. Is there something that you're having trouble with? Force pushing to branches that have been shared publicly is generally frowned upon because it rewrites the git repo's history, making it difficult to understand what changes have been made and when. Back in the day, I kept this printed out next to my desk for when I got into sticky situations. Let me know if I can help, we can touch base tomorrow as needed. |
I was doing further development and forgot that those changes were showing up here. Kubernetes website instructs users to force push with each new commit on a PR (they want 1 commit for each PR), so it's a bit of a learning curve for a different project. |
Ahh I see, glad to hear all's well. We're getting close here, giving this another look now |
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 think we're really close, couple minor suggestions and I think we'll be good to go!
Co-authored-by: Alex Kestner <[email protected]>
Ship it! |
…fied by default (aws#527)
Issue, if available: N/A
Description of changes:
paths to review:
website/content/en/docs/getting-started/_index.md
do not review paths:
docs/*
I respectfully submit that housing the HTML here makes PRs very confusing. Additionally, this code only works on local previews, not GH pages.
Screenshots
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.