-
Notifications
You must be signed in to change notification settings - Fork 16.8k
[stable/efs-provisioner] Chart for efs-provisioner, a Kubernetes external-storage provisioner #3233
Conversation
Hi @unguiculus, thanks for reviewing this chart. I just added some improvements. The chart has been working well for me. |
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.
Please apply current RBAC best practices.
https://github.com/kubernetes/helm/blob/master/docs/chart_best_practices/rbac.md
incubator/efs-provisioner/Chart.yaml
Outdated
- https://github.com/kubernetes-incubator/external-storage/tree/master/aws/efs | ||
- https://github.com/mirror/busybox | ||
maintainers: | ||
- name: Aaron Roydhouse |
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.
Use github username.
metadata: | ||
name: {{ template "efs-provisioner.fullname" . }} | ||
labels: | ||
app: {{ template "efs-provisioner.fullname" . }} |
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.
The app
label should be {{ template "efs-provisioner.name" . }}
. Apply throughout the chart.
@@ -0,0 +1,60 @@ | |||
kind: Deployment | |||
apiVersion: extensions/v1beta1 |
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.
apps/v1beta2
release: "{{ .Release.Name }}" | ||
heritage: "{{ .Release.Service }}" | ||
spec: | ||
replicas: {{ default 1 .Values.replicaCount }} |
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.
Don't use the default
function. Defaults are in values.yaml
anyways.
template: | ||
metadata: | ||
labels: | ||
app: {{ template "efs-provisioner.fullname" . }} |
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.
Add release
label.
/ok-to-test |
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
/test pull-charts-e2e |
@unguiculus I made some changes but the CLA label has been reverted. Nothing has changed, I'm still signed up (I logged in to check, still says signed up) and my email hasn't changed. |
@whereisaaron Your CLA is fine. I guess it's the commit @gabeduke pusahed to your branch. Can you rebase and clean up the history? |
Oops! @whereisaaron I just signed the CLA. Can this be retested, or is there a wait period for that to be accepted? |
@gabeduke It has already been tested. Problems need to be fixed. |
@unguiculus updated and squashed it. The release test will not succeed, as the chart has the external dependency on an EFS filesystem being available to mount. |
@whereisaaron Please check RBAC best practices again. You need to add a partial for the service account. |
Thanks @unguiculus but I am not sure what you mean by a 'partial'? I checked the Best Practices page but it doesn't mention what a 'partial' is? I saw there was a handy new helper template for Service Account names though, so I adopted that and tested all the |
## https://github.com/kubernetes-incubator/external-storage/tree/master/aws/efs#deployment | ||
## | ||
efsProvisioner: | ||
efsFileSyteId: fs-12345678 |
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.
efsFileSyteId
-> efsFileSystemId
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 @nazarewk! I have made the change.
@whereisaaron A "partial" is a named template, so you got it right. |
I'd suggest you move the chart to stable before we merge. |
Thanks @unguiculus I've moved it. |
stable/efs-provisioner/README.md
Outdated
At a minimum you must the supply the EFS file system ID and the AWS region | ||
|
||
``` | ||
helm install incubator/efs-provisioner --set efsFileSystemId=fs-12345678 --set awsRegion=us-east-2 |
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.
Since you've moved to stable, can you please update 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.
Ah yes. Good point @jeyglk. I'll do that and search for other refs.
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.
Done 👍
stable/efs-provisioner/README.md
Outdated
All the values documented below and by `helm inspect values`. | ||
|
||
``` | ||
helm inspect values incubator/efs-provisioner |
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.
ditto
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.
Done 👍
…rnal-storage provisioner
@unguiculus moved, squashed, and rebased |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: unguiculus, whereisaaron The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…labels * kubernetes/master: (411 commits) [stable/efs-provisioner] Chart for efs-provisioner, a Kubernetes external-storage provisioner (helm#3233) [stable/filebeat] filebeat fixes (helm#6332) [stable/stolon] Add support for priorityClasses (helm#6607) [stable/external-dns] Add support for priorityClasses (helm#6606) [stable/minio] Add support for priorityClasses (helm#6604) [stable/cluster-autoscaler] Add support for priorityClasses (helm#6603) [stable/oauth2-proxy] Add support for priorityClasses (helm#6586) [stable/elasticsearch-exporter] add support for priorityClasses (helm#6584) [stable/traefik] adding support for traefik wildcard certificates (helm#6015) gcloud-sqlproxy: add tolerations and affinity to the Deployment (helm#6495) Review readme (helm#6399) [stable/mongodb-replicaset] Prometheus Metrics export (helm#6282) [stable/artifactory-ha] Typo fix: livessProbed->livenessProbed (helm#6462) [stable/artifactory] livessProbed->livenessProbed (helm#6461) [incubator/kube-spot-termination-notice-handler] Add Support for Tolerations (helm#5813) [stable/kanister-operator] RBAC changes and kanister profile creation (helm#6280) fix redis-ha NOTE.txt, stable/redis-ha don't create master-0 pod (helm#6131) [stable/concourse] add support for custom envvars for the web containers (helm#6441) upgrade to latest prometheus release 2.3.2 and alertmanager 0.15.1 (helm#6623) cert-manager: fast-forward to upstream 777ce6f4 (helm#6625) ...
Great! Thanks for your help and reviews @unguiculus @jeyglk @gabeduke. |
…rnal-storage provisioner (helm#3233) Signed-off-by: Jakob Niggel <info@jakobniggel.de>
…rnal-storage provisioner (helm#3233)
This chart deploys the EFS external storage provisioner, which is a Kubernetes incubator project:
https://github.com/kubernetes-incubator/external-storage/tree/master/aws/efs
The chart follows the deployment manifests recommended by the
efs-provisioner
authors and I believe follows the best practices from the Helm documentation, and chart technical requirements.The is no
appversion
in theChart.yaml
. As this is deploying a incubator project the authors' manifests recommend deploying 'latest', and 'latest' is not semVer compliant :-)To test this you need a Kubernetes 1.8+ cluster running on AWS with and EFS file system available.