-
Notifications
You must be signed in to change notification settings - Fork 115
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
Improve readme #120
Improve readme #120
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.
I love the new README 😍 Thanks for working on it.
I have just a few suggestions.
README.md
Outdated
- The target namespace must already exist in the target context | ||
- `ENV['GOOGLE_APPLICATION_CREDENTIALS']` must point to the credentials for an authenticated service account if your user's auth provider is GCP | ||
- `ENV['ENVIRONMENT']` must be set to use the default template path (`config/deploy/$ENVIRONMENT`) in the absence of the `--template-dir=DIR` option | ||
- `ENV[REVISION]` **(required)**: the SHA of the commit you are deploying. Will be exposed to your ERB templates as `current_sha`. |
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.
ENV[REVISION]
may be unclear for someone outside of Ruby because AFAIK most Linux tools refer to env variables like $REVISION
README.md
Outdated
|
||
:closed_lock_with_key: [Creates Kubernetes secrets from encrypted EJSON](#deploying-kubernetes-secrets-from-ejson), which you can safely commit to your repository | ||
|
||
:running: [Runs tasks at the beginning of the deploy](#deploy-time-tasks) using pods (example use case: Rails migrations) |
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 "using bare pods"?
README.md
Outdated
*Requirements:* | ||
|
||
* The pod's name should include `<%= deployment_id %>` to ensure that a unique name will be used on every deploy (the deploy will fail if a pod with the same name already exists). | ||
* The pod's `spec.restartPolicy` must be set to `Never` |
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'd mention why it has be be Never
(because it's a one-off task)
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.
It will help people to learn more about restartPolicy
, not just blindly following the instruction
README.md
Outdated
|
||
## Highlights | ||
|
||
:eyes: Watches the changes you requested to make sure they complete successfully. |
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 "to make sure they rollout successfully"?
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.
For the deployment process, the definition of "complete" may be unclear
|
||
|
||
|
||
## Table of contents |
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 love the new structure!
|
||
[![Build status](https://badge.buildkite.com/0f2d4956d49fbc795f9c17b0a741a6aa9ea532738e5f872ac8.svg?branch=master)](https://buildkite.com/shopify/kubernetes-deploy-gem) | ||
`kubernetes-deploy` is a command line tool that helps you ship changes to a Kubernetes namespace and understand the result. At Shopify, we use it within our much-beloved, open-source [Shipit](https://github.com/Shopify/shipit-engine#kubernetes) deployment app. |
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'd also mention why a separate deployment tool is needed for k8s (for instance because running kubectl apply
from CI doesn't give a lot of control over it)
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 suggestion. I'll take a stab at it!
README.md
Outdated
* Your cluster must be running Kubernetes v1.6.0 or higher | ||
* Each app must have a deploy directory containing its Kubernetes templates (see [Templates](#templates)) | ||
* You must remove the` kubectl.kubernetes.io/last-applied-configuration` annotation from any resources in the namespace that are not included in your deploy directory. If you do not, kubernetes-deploy will delete them. | ||
* Each app managed by `kubernetes-deploy` must have its own exclusive namespace. (This requirement can be bypassed with the `--no-prune` option, but it is not recommended.) |
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 exclusive namespace
=> exclusive Kubernetes namespace
?
README.md
Outdated
|
||
* Your cluster must be running Kubernetes v1.6.0 or higher | ||
* Each app must have a deploy directory containing its Kubernetes templates (see [Templates](#templates)) | ||
* You must remove the` kubectl.kubernetes.io/last-applied-configuration` annotation from any resources in the namespace that are not included in your deploy directory. If you do not, kubernetes-deploy will delete them. |
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.
Just curious, do you remove them by editing YAMLs? I never saw this annotation.
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.
That is the annotation that gets added when you create/update things via kubectl apply
--I should probably mention that explicitly. It's what the pruning feature uses to identify which resources kubectl is authorized to manage. The easiest way to remove it is kubectl edit
(and then remember not to kubectl apply
the thing in the future!).
README.md
Outdated
|
||
:eyes: Watches the changes you requested to make sure they complete successfully. | ||
|
||
:interrobang: Provides debug information for changes that failed. |
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 feature list!
Updated! Please take another look, particularly at the new intro para. |
@KnVerey I really like this. I think some future work could be:
None of this needs to be incorporated into this PR, just some ideas :) |
I don't think there's a way to PR this change, but wdyt about updating the repo's description? Currently it is:
I'm thinking of pulling one of these lines from this PR:
|
My vote^ |
|
||
## Installation | ||
Especially in a CI/CD environment, we need a clear, actionable pass/fail result for each deploy. Providing this was the foundational goal of `kubernetes-deploy`, which has grown to support the following core features: |
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.
One nit, otherwise looks great 👌
README.md
Outdated
|
||
All templates must be YAML formatted. You can also use ERB. The following local variables will be available to your ERB templates by default: | ||
|
||
* `current_sha`: The value of `ENV['REVISION']` |
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 stick with $REVISION
that's used above
*Options:* | ||
|
||
- `--bindings=BINDINGS`: Makes additional variables available to your ERB templates. For example, `kubernetes-deploy my-app cluster1 --bindings=color=blue,size=large` will expose `color` and `size`. | ||
- `--verbose-log-prefix`: Prefixes each log line with `[#{context}][#{namespace}][#{severity}][#{datetime}]` instead of just `[#{severity}][#{datetime}]`. Useful if you're running multiple concurrent deploys in Shipit. |
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 in the future we refer to kubernetes-deploy --help
as the full source of documentation of extra flags, instead of the README
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.
(because IMO things like --verbose-log-prefix
are not going to be used frequently enough to highlight in the README)
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 a good idea.
cc @alindeman, who I've spoken to about this project before, and can provide some truly fresh 👀 from outside Shopify on the documentation |
I'm going to merge this now. I'll make a follow-up that adds a gif, improves |
@Shopify/cloudplatform @kirs @xldenis
This PR improves the repo's README. Until now, its secondary features were far-better documented than the main ones! I'm giving a presentation on this gem at a meetup early next week, so I would like for these improvements to be in before we (hopefully) get more people interested in reading about it.
Helps with #105