Refactor: adopt dependency injection pattern and extract cloud property #1094
Conversation
k8s-ci-robot
added
kind/cleanup
k8s-ci-robot
added
the
size/L
| userAgent := blob.GetUserAgent(driverOptions.DriverName, *customUserAgent, *userAgentSuffix) | ||
| klog.V(2).Infof("driver userAgent: %s", userAgent) | ||
|
|
||
| cloud, err := blob.GetCloudProvider(*kubeconfig, driverOptions.NodeID, *cloudConfigSecretName, *cloudConfigSecretNamespace, userAgent, *allowEmptyCloudConfig, *kubeAPIQPS, *kubeAPIBurst) |
There was a problem hiding this comment.
why not put all these into DriverOptions? that's the reason why DriverOptions exists, it could save parameter num
*cloudConfigSecretName, *cloudConfigSecretNamespace, userAgent, *allowEmptyCloudConfig, *kubeAPIQPS, *kubeAPIBurst
There was a problem hiding this comment.
It's a little bit complicated to add mock for GetCloudProvider
There was a problem hiding this comment.
but why move part of the options as parameters? this is not consistent
There was a problem hiding this comment.
Should I move other dependencies out of newDriver function? In that case that would be easier to compose driver object and there is no need to implement FakeDriver.
| } | ||
| }, | ||
| }, | ||
| { |
There was a problem hiding this comment.
This case is deleted because fake driver has got required capabilities.
There was a problem hiding this comment.
what's the result of this test now? invalid create volume req" ?
There was a problem hiding this comment.
original test is for testing invalid create volume req, not capabilities
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andyzhangx, MartinForReal 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
|
/retest |
|
/test pull-blob-csi-driver-e2e |
| } | ||
| }, | ||
| }, | ||
| { |
There was a problem hiding this comment.
original test is for testing invalid create volume req, not capabilities
| }, | ||
| }, | ||
| { | ||
| name: "invalid delete volume req", |
There was a problem hiding this comment.
could you also add this ut back?
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
adopt dependency injection pattern and extract cloud property from driver factory
Which issue(s) this PR fixes:
Fixes #
Requirements:
Special notes for your reviewer:
Release note: