Fix go lint failures in volume scheduling packages #77442

cofyc · 2019-05-04T15:10:44Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:

pkg/controller/volume/persistentvolume/testing
pkg/controller/volume/scheduling

Which issue(s) this PR fixes:

Fixes #77084

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

cofyc · 2019-05-04T15:11:28Z

pkg/controller/volume/persistentvolume/testing/testing.go

-var VersionConflictError = errors.New("VersionError")
+// ErrVersionConflict is the error returned when resource version of requested
+// object conflicts with the object in storage.
+var ErrVersionConflict = errors.New("VersionError")


golint: error var VersionConflictError should have name of the form ErrFoo

cofyc · 2019-05-04T15:11:50Z

pkg/controller/volume/persistentvolume/testing/testing.go

@@ -299,12 +297,6 @@ func (r *VolumeReactor) getWatches(gvr schema.GroupVersionResource, ns string) [
 	return watches
 }

-func (r *VolumeReactor) ChangedSinceLastSync() int {


Duplicate function

cofyc · 2019-05-04T15:12:31Z

pkg/controller/volume/scheduling/scheduler_assume_cache.go

@@ -127,7 +127,8 @@ func (c *assumeCache) objInfoIndexFunc(obj interface{}) ([]string, error) {
 	return c.indexFunc(objInfo.latestObj)
 }

-func NewAssumeCache(informer cache.SharedIndexInformer, description, indexName string, indexFunc cache.IndexFunc) *assumeCache {
+// NewAssumeCache creates an assume cache for genernal objects.
+func NewAssumeCache(informer cache.SharedIndexInformer, description, indexName string, indexFunc cache.IndexFunc) AssumeCache {


exported func NewAssumeCache returns unexported type *scheduling.assumeCache, which can be annoying to use
I guess it's better to expose interface instead struct, so I changed the function to return interface

cofyc · 2019-05-04T15:15:04Z

pkg/controller/volume/scheduling/scheduler_assume_cache.go

 func NewPVAssumeCache(informer cache.SharedIndexInformer) PVAssumeCache {
-	return &pvAssumeCache{assumeCache: NewAssumeCache(informer, "v1.PersistentVolume", "storageclass", pvStorageClassIndexFunc)}
+	return &pvAssumeCache{NewAssumeCache(informer, "v1.PersistentVolume", "storageclass", pvStorageClassIndexFunc).(*assumeCache)}


because it returns an interface now, we need to cast it to *assumeCache back here
It feels weird but we cannot use AssumeCache interface in pvAssumeCache struct because private functions of assumeCache are required in testing pvAssumeCache and pvcAssumeCache

Hm can the unit tests do one more level of casting to get the 'assumeCache'?

Looking at L348, I'm not sure we need to copy private 'assumeCache' into the struct. It looks like we only use interface functions in the PV and PVC implementations

can the unit tests do one more level of casting to get the 'assumeCache'?

At before, I thought we need two variables to access pv/pvc)AssumeCache and assumeCache with which the code will be tedious, but I found in testing we didn't need to access private methods of (pv/pvc)AssumeCache.

It's a good idea. I pushed a new commit to address this: 4abd730

cofyc · 2019-05-04T15:16:17Z

pkg/controller/volume/scheduling/scheduler_assume_cache.go

 func NewPVCAssumeCache(informer cache.SharedIndexInformer) PVCAssumeCache {
-	return &pvcAssumeCache{assumeCache: NewAssumeCache(informer, "v1.PersistentVolumeClaim", "namespace", cache.MetaNamespaceIndexFunc)}
+	return &pvcAssumeCache{NewAssumeCache(informer, "v1.PersistentVolumeClaim", "namespace", cache.MetaNamespaceIndexFunc).(*assumeCache)}


same as NewPVAssumeCache

cofyc · 2019-05-04T15:16:30Z

pkg/controller/volume/scheduling/scheduler_assume_cache_test.go

@@ -109,13 +109,13 @@ func TestAssumePV(t *testing.T) {

 	for name, scenario := range scenarios {
 		cache := NewPVAssumeCache(nil)
-		internal_cache, ok := cache.(*pvAssumeCache)
+		internalCache, ok := cache.(*pvAssumeCache)


don't use underscores in Go names; var internal_cache should be internalCache

cofyc · 2019-05-04T15:18:06Z

pkg/controller/volume/persistentvolume/testing/testing.go

 		}
+		klog.V(4).Infof("GetVolume: volume %s not found", name)


if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)
it applies to all if else cases

cofyc · 2019-05-04T15:18:45Z

pkg/controller/volume/scheduling/scheduler_binder_fake.go

@@ -18,7 +18,7 @@ package scheduling

 import "k8s.io/api/core/v1"

-type FakeVolumeBinderConfig struct {
+type fakeVolumeBinderConfig struct {


unexported it then we don't need to comment it

cofyc · 2019-05-04T15:19:32Z

pkg/controller/volume/scheduling/scheduler_binder_fake.go

 // topology-aware volume binding decisions.
-func NewFakeVolumeBinder(config *FakeVolumeBinderConfig) *FakeVolumeBinder {
-	return &FakeVolumeBinder{
+func NewFakeVolumeBinder(config *fakeVolumeBinderConfig) SchedulerVolumeBinder {


return exported interface

cofyc · 2019-05-04T15:41:51Z

/assign @dims @fejta
for hack/.golint_failures approval
/assign @msau42
for reviewing

- pkg/controller/volume/persistentvolume/testing - pkg/controller/volume/scheduling

fejta

/lgtm
/approve

msau42 · 2019-05-07T03:03:02Z

/lgtm
/approve

msau42 · 2019-05-07T03:03:11Z

/priority important-longterm

k8s-ci-robot · 2019-05-07T03:03:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cofyc, fejta, msau42

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:

~~hack/OWNERS~~ [fejta]
~~pkg/controller/volume/persistentvolume/OWNERS~~ [msau42]
~~pkg/controller/volume/scheduling/OWNERS~~ [cofyc,msau42]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

cofyc commented May 4, 2019

View reviewed changes

k8s-ci-robot requested review from rootfs and SataQiu May 4, 2019 15:11

cofyc commented May 4, 2019

View reviewed changes

cofyc changed the title ~~Fix go lint failures in a few packages~~ Fix go lint failures in volume scheduling packages May 4, 2019

cofyc force-pushed the fix77084 branch 2 times, most recently from af7237b to 529ebc2 Compare May 4, 2019 15:39

k8s-ci-robot assigned dims, fejta and msau42 May 4, 2019

Fix go lint failures in a few packages

3315474

- pkg/controller/volume/persistentvolume/testing - pkg/controller/volume/scheduling

cofyc force-pushed the fix77084 branch from 529ebc2 to 3315474 Compare May 5, 2019 01:45

fejta reviewed May 7, 2019

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 7, 2019

Do one more level of casting to get the 'assumeCache'

4abd730

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 7, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 7, 2019

k8s-ci-robot merged commit 43284ec into kubernetes:master May 7, 2019

cofyc deleted the fix77084 branch September 2, 2019 05:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix go lint failures in volume scheduling packages #77442

Fix go lint failures in volume scheduling packages #77442

cofyc commented May 4, 2019

cofyc May 4, 2019

cofyc May 4, 2019

cofyc May 4, 2019 •

edited

cofyc May 4, 2019 •

edited

msau42 May 6, 2019

cofyc May 7, 2019

cofyc May 4, 2019

cofyc May 4, 2019

cofyc May 4, 2019

cofyc May 4, 2019

cofyc May 4, 2019

cofyc commented May 4, 2019

fejta left a comment

msau42 commented May 7, 2019

msau42 commented May 7, 2019

k8s-ci-robot commented May 7, 2019

Fix go lint failures in volume scheduling packages #77442

Fix go lint failures in volume scheduling packages #77442

Conversation

cofyc commented May 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cofyc May 4, 2019 • edited

Choose a reason for hiding this comment

cofyc May 4, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cofyc commented May 4, 2019

fejta left a comment

Choose a reason for hiding this comment

msau42 commented May 7, 2019

msau42 commented May 7, 2019

k8s-ci-robot commented May 7, 2019

cofyc May 4, 2019 •

edited

cofyc May 4, 2019 •

edited