deploy: add README, fix typos #124
Conversation
The csi-hostpath-snapshotter.yaml was misspelled when creating the file. The upstream driver-registrar RBAC file uses "csi-driver-registrar" as service account name and therefore the hostpath plugin should also use that instead of the (now obsolete) "csi-node-sa" name from the Kubernetes E2E testing. This dependency on the upstream RBAC rules should be mentioned. For the sake of avoiding yet another copy of URLs the new README just refers to other documentation.
k8s-ci-robot
added
size/XS
|
This blocks the merging of kubernetes/kubernetes#71703 because I need an upstream copy of the hostpathplugin .yaml that works with the upstream driver-registrar RBAC .yaml. |
| @@ -0,0 +1,6 @@ | |||
| The .yaml files in this directory depend on the RBAC files that were | |||
There was a problem hiding this comment.
I think the rbac rules should be copied into examples/hostpath so that a hostpath deployment can be done using the YAML files. The docs will eventually point to the hostpath repos.
There was a problem hiding this comment.
No, please not. Having to keep files in sync in different repos is exactly the kind of maintenance headache that we are currently trying to get rid of.
The Example.md is self-contained without copies because it references upstream files via URL. I have a PR pending for Kubernetes E2E which achieves the same (kubernetes/kubernetes#71703). Let's not go in the other direction here.
The deployment here won't be complete, but it doesn't have to be. That's what the Example.md is for - IMHO.
There was a problem hiding this comment.
FWIW, I personally would have preferred a solution where all relevant .yaml files are in the same repo because then they can be updated and tested together. But it was decided that each component maintainer is also responsible for maintaining the corresponding .yaml files (in particular the RBAC files), which is why we now move the individual files away from centralized places (docs, kubernetes E2E) into individual repos.
There was a problem hiding this comment.
Agree with your last comment re: having all yamls in the same repo. My thought was to have all yamls that are necessary to standup a hostpath instance under hostpath/example instead of docs/examples to minimize. I think when the config yamls are located in docs/examples, it may not be clear that they need updated as hostpath changes.
There was a problem hiding this comment.
Moving from kubernetes-csi/docs is good. Then eventually kubernetes-csi/docs will just reference files elsewhere by URL.
But do you really mean all yamls, including the RBAC ones from driver-registrar, external-attacher and external-provisioner? That goes against the decision made earlier, so before you do that, discuss it in the CSI standup.
In the meantime, can we merge this PR?
There was a problem hiding this comment.
We should leave RBAC files in sidecar container repos (external-provisioner, external-attacher, etc.).
Each driver can provide it's own YAMLs for DeamonSet/ReplicaSet, along with a deployment script that deploys all the YAMLs (including the RBAC ones).
|
/lgtm |
|
/approve |
| @@ -0,0 +1,6 @@ | |||
| The .yaml files in this directory depend on the RBAC files that were | |||
There was a problem hiding this comment.
We should leave RBAC files in sidecar container repos (external-provisioner, external-attacher, etc.).
Each driver can provide it's own YAMLs for DeamonSet/ReplicaSet, along with a deployment script that deploys all the YAMLs (including the RBAC ones).
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pohly, saad-ali, vladimirvivien 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 |
k8s-ci-robot
added
the
approved
The csi-hostpath-snapshotter.yaml was misspelled when creating the
file. The upstream driver-registrar RBAC file uses
"csi-driver-registrar" as service account name and therefore the
hostpath plugin should also use that instead of the (now obsolete)
"csi-node-sa" name from the Kubernetes E2E testing.
This dependency on the upstream RBAC rules should be mentioned. For
the sake of avoiding yet another copy of URLs the new README just
refers to other documentation.
/cc @vladimirvivien