Refactor the code #135
Refactor the code #135
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: leakingtapan 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
approved
Pull Request Test Coverage Report for Build 212
💛 - Coveralls |
| } | ||
|
|
||
| // controller capability | ||
| controllerCaps = []csi.ControllerServiceCapability_RPC_Type{ |
There was a problem hiding this comment.
I'm not sure if making these fields global is a better practice. It'd make it difficult to create a driver with custom capabilities for testing, for example.
Another option would be to create functions to set up and return the caps, and use these function in New*Driver().
If we want to keep it like this, we may want to name these vars like defaultVolume... and use these names in the comments (e.g., defaultVolumeCapabilities represents the EBS volume....
There was a problem hiding this comment.
I'm not sure if making these fields global is a better practice. It'd make it difficult to create a driver with custom capabilities for testing, for example.
I feel we will never create a CSI EBS driver with a different capability eg, volume capability of MULTI_NODE_MULTI_WRITER since that is limited by EBS; or we will create a driver whose controller service capability is configurable, eg CREATE_DELETE_VOLUME is configurable since we always want dynamic provisioning.
I kept variable name but updated the comments of those variable though.
There was a problem hiding this comment.
I don't feel like moving those vars to a global scope would make the code better, even though they might not be ever changed.
I agree that initializing them in NewDriver feels messy, but I'd rather create functions that return the default values and use those functions in NewDriver (if I'm not mistaken GCP driver does something like that).
However, I don't want to block this PR from being merged, so feel free to merge it if you think it's OK as is.
leakingtapan
force-pushed
the
next
branch
2 times, most recently
from
1a65009 to
eea62c5
Compare
1. Split `NewDriver` into two methods `NewDriver` and `NewFakeDriver`. So that one only takes in endpoint and serves as production uses and the other create driver with mock dependencies 2. Move controller capability, node capability and volume capability out of driver struct since they are constant that should never change for a specific version of driver
|
@bertinatto yes, its going to be merged into master |
|
@leakingtapan: ah, OK. I just noticed that |
NewDriverinto two methodsNewDriverandNewMockDriver.So that one is only takes in endpoint and serves as production uses and
the other create driver with mock dependencies
of driver struct since they are constant that should never change for a
specific version of driver