-
Notifications
You must be signed in to change notification settings - Fork 260
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
Changes from testing before first release #24
Changes from testing before first release #24
Conversation
currently have this deployed in Kind, without visible errors: csi-nodeplugin-nfsplugin container node-driver-registrar
csi-nodeplugin-nfsplugin container nfs
csi-attacher-nfsplugin container csi-attacher
csi-attacher-nfsplugin container nfs
|
@@ -16,12 +16,15 @@ spec: | |||
|
|||
--- | |||
kind: StatefulSet | |||
apiVersion: apps/v1beta1 | |||
apiVersion: apps/v1 |
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.
https://kubernetes.io/blog/2019/07/18/api-deprecations-in-1-16/
since 1.16, those apps/v1beta1 and v1beta2 are deprecated
lgtm and tests out when exercised by manila-csi |
/assign msau42 |
@@ -16,12 +16,15 @@ spec: | |||
|
|||
--- | |||
kind: StatefulSet | |||
apiVersion: apps/v1beta1 | |||
apiVersion: apps/v1 | |||
metadata: | |||
name: csi-attacher-nfsplugin |
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 actually don't need attacher anymore starting in 1.14. We can set CSIDriver.attachRequired = false.
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.
perfect, thanks, removed the attacher and added CSIDriver in 50d5f44
and starting to have a tiny idea how does CSI work, finally :)
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, msau42!
With the latest commit this still checks out as I use it from manila-csi ...
tested the CSI plugin and it seems to be working. The docs are also fairly accurate although I did take a bit of a detour using https://github.com/kubernetes-sigs/kind and https://github.com/rootfs/nfs-ganesha-docker. This might be worth appending to the docs at some point as alternative and less involved route for contributors to setup testing env. From my perspective, this is ok to be released, what do you think @tombarron, @msau42 ? |
Seems good to me but @msau42 will know more :D |
@@ -40,7 +40,7 @@ const ( | |||
) | |||
|
|||
var ( | |||
version = "1.0.0-rc2" | |||
version = "1.0.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.
also noticed this while glancing through the code. Probably worth at some point deriving this from git tag/sha in a similar fashion that kubectl does during build process?
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 csi build tools does support injecting git tag into the version: https://github.com/kubernetes-csi/csi-release-tools/blob/master/build.make#L77
We just need to fix this up to use that. We can do that in a followup
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: msau42, wozniakjan 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 |
Change owners
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Eventually this should lead to a fully tested release of v1.0.0 with up-to-date documentation.Currently this is a placeholder for WIP to inform the status and the fact it hasn't been abandoned, just lacking free time to finish testing this :)Changes that spawned up from testing, confirmed to be working fine and able to bind and mount PVC in multiple pods and read/write simultaneously.
Which issue(s) this PR fixes:
Allow for first initial release #4
Special notes for your reviewer:
Patience, my dear Watson 🕵️♂️
Does this PR introduce a user-facing change?:
fyi: @msau42, @tombarron