Some minor fixes for warnings while running the tests #68481

duglin · 2018-09-10T18:09:53Z

Just some minor things I noticed while running the tests today - nothing big.

Signed-off-by: Doug Davis dug@us.ibm.com

k8s-ci-robot · 2018-09-10T18:09:54Z

@duglin: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

duglin · 2018-09-10T18:10:16Z

cmd/kubeadm/app/phases/certs/certs.go

@@ -110,7 +110,7 @@ func CreateCACertAndKeyFiles(certSpec *KubeadmCert, cfg *kubeadmapi.InitConfigur
 	if certSpec.CAName != "" {
 		return fmt.Errorf("This function should only be used for CAs, but cert %s has CA %s", certSpec.Name, certSpec.CAName)
 	}
-	glog.V(1).Infoln("creating a new certificate authority for %s", certSpec.Name)
+	glog.V(1).Infof("creating a new certificate authority for %s", certSpec.Name)


this one complained about having %s but not using Infof

duglin · 2018-09-10T18:10:27Z

pkg/util/ipset/ipset.go

@@ -485,7 +485,7 @@ func validateProtocol(protocol string) bool {
 	if protocol == ProtocolTCP || protocol == ProtocolUDP || protocol == ProtocolSCTP {
 		return true
 	}
-	glog.Errorf("Invalid entry's protocol: %s, supported protocols are [%s, %s]", protocol, ProtocolTCP, ProtocolUDP, ProtocolSCTP)
+	glog.Errorf("Invalid entry's protocol: %s, supported protocols are [%s, %s, %s]", protocol, ProtocolTCP, ProtocolUDP, ProtocolSCTP)


too few %s's

#68263 fixes this but it's ok.

duglin · 2018-09-10T18:10:51Z

pkg/volume/azure_dd/azure_dd.go

@@ -188,7 +188,7 @@ func getMaxDataDiskCount(instanceType string, sizeList *[]compute.VirtualMachine
 			continue
 		}
 		if strings.ToUpper(*size.Name) == vmsize {
-			glog.V(2).Infof("got a matching size in getMaxDataDiskCount, Name: %s, MaxDataDiskCount: %s", *size.Name, *size.MaxDataDiskCount)
+			glog.V(2).Infof("got a matching size in getMaxDataDiskCount, Name: %v, MaxDataDiskCount: %v", *size.Name, *size.MaxDataDiskCount)


wrong datatype on the %'s - they're numeric not string

We should keep the first %s, *size.Name is string.

@duglin is this a linter error?
the rest LGTM.

%v works just fine for strings, but I'll switch it back to %s.
I don't think it was a linter error, is linter run as part of "make test" ?
The console showed this:

# k8s.io/kubernetes/cmd/kubeadm/app/phases/certs cmd/kubeadm/app/phases/certs/certs.go:113: Verbose.Infoln call has possible formatting directive %s

is linter run as part of "make test"

i don't think so.

duglin · 2018-09-10T18:13:05Z

/release-note-none

duglin · 2018-09-10T20:12:19Z

easy review
ping @detiber @islinwb @dims

neolit123

/kind cleanup

neolit123 · 2018-09-11T01:23:28Z

pkg/volume/azure_dd/azure_dd.go

@@ -188,7 +188,7 @@ func getMaxDataDiskCount(instanceType string, sizeList *[]compute.VirtualMachine
 			continue
 		}
 		if strings.ToUpper(*size.Name) == vmsize {
-			glog.V(2).Infof("got a matching size in getMaxDataDiskCount, Name: %s, MaxDataDiskCount: %s", *size.Name, *size.MaxDataDiskCount)
+			glog.V(2).Infof("got a matching size in getMaxDataDiskCount, Name: %v, MaxDataDiskCount: %v", *size.Name, *size.MaxDataDiskCount)


@duglin is this a linter error?
the rest LGTM.

islinwb · 2018-09-11T01:47:02Z

/lgtm

Signed-off-by: Doug Davis <dug@us.ibm.com>

duglin · 2018-09-11T03:38:59Z

addressed comment

neolit123 · 2018-09-11T03:46:16Z

thanks
/lgtm

duglin · 2018-09-11T03:48:43Z

/test pull-kubernetes-e2e-gce-device-plugin-gpu

duglin · 2018-09-11T20:52:54Z

ping @neolit123

neolit123 · 2018-09-11T20:58:31Z

@duglin thanks for the ping.
one of the milestone maintainers should add a milestone if this is to be merged for 1.12.
otherwise it has to wait after 1.12 - this is more likely given it requires approval by multiple parties.

/assign @fabriziopandini

duglin · 2018-09-11T21:01:08Z

sounds fair, definitely not critical for 1.12

duglin · 2018-09-25T13:20:24Z

ok to merge now that there's a thaw?

neolit123 · 2018-09-25T13:28:05Z

/lgtm

neolit123 · 2018-09-28T22:07:37Z

/assign @fabriziopandini @timothysc
PR is LGTM, needs approve.

timothysc

/approve

duglin · 2018-11-04T17:00:09Z

any chance of getting this one in?

neolit123 · 2018-11-04T17:13:22Z

needs approve from @kubernetes/sig-azure too.
/assign @brendandburns

andyzhangx

/lgtm
/approve

timothysc

/lgtm
/approve

duglin · 2018-11-08T19:49:38Z

@brendanburns any chance you can review this? This minor PR has been there for a while now and I'd like to get it into 1.13

k8s-ci-robot · 2018-11-09T00:24:22Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: andyzhangx, duglin, timothysc

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:

~~cmd/kubeadm/OWNERS~~ [timothysc]
pkg/util/ipset/OWNERS
~~pkg/volume/azure_dd/OWNERS~~ [andyzhangx]

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

duglin · 2018-11-09T01:17:49Z

/test pull-kubernetes-e2e-kops-aws

duglin · 2018-11-09T01:34:19Z

/test pull-kubernetes-e2e-gce
/test pull-kubernetes-e2e-kops-aws

duglin · 2018-11-09T02:43:40Z

all tests are passing - who can push this over the finish line?

duglin commented Sep 10, 2018

View reviewed changes

k8s-ci-robot requested review from detiber and islinwb September 10, 2018 18:10

k8s-ci-robot added area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/storage Categorizes an issue or PR as relevant to SIG Storage. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 10, 2018

duglin commented Sep 10, 2018

View reviewed changes

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 10, 2018

neolit123 reviewed Sep 11, 2018

View reviewed changes

k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Sep 11, 2018

k8s-ci-robot assigned islinwb Sep 11, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 11, 2018

Some minor fixes for warnings while running the tests

9865aef

Signed-off-by: Doug Davis <dug@us.ibm.com>

duglin force-pushed the minorFixes branch from d70d64a to 9865aef Compare September 11, 2018 03:38

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 11, 2018

k8s-ci-robot assigned neolit123 Sep 11, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 11, 2018

k8s-ci-robot assigned fabriziopandini Sep 11, 2018

k8s-ci-robot assigned timothysc Sep 28, 2018

timothysc approved these changes Oct 1, 2018

View reviewed changes

k8s-ci-robot assigned brendandburns and andyzhangx Nov 4, 2018

andyzhangx approved these changes Nov 5, 2018

View reviewed changes

timothysc approved these changes Nov 5, 2018

View reviewed changes

timothysc added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 9, 2018

k8s-ci-robot merged commit b3a160d into kubernetes:master Nov 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some minor fixes for warnings while running the tests #68481

Some minor fixes for warnings while running the tests #68481

duglin commented Sep 10, 2018

k8s-ci-robot commented Sep 10, 2018

duglin Sep 10, 2018

duglin Sep 10, 2018

islinwb Sep 11, 2018

duglin Sep 10, 2018

linyouchong Sep 11, 2018 •

edited

neolit123 Sep 11, 2018

duglin Sep 11, 2018

neolit123 Sep 11, 2018

duglin commented Sep 10, 2018

duglin commented Sep 10, 2018

neolit123 left a comment

neolit123 Sep 11, 2018

islinwb commented Sep 11, 2018

duglin commented Sep 11, 2018

neolit123 commented Sep 11, 2018

duglin commented Sep 11, 2018

duglin commented Sep 11, 2018

neolit123 commented Sep 11, 2018

duglin commented Sep 11, 2018

duglin commented Sep 25, 2018

neolit123 commented Sep 25, 2018

neolit123 commented Sep 28, 2018

timothysc left a comment

duglin commented Nov 4, 2018

neolit123 commented Nov 4, 2018

andyzhangx left a comment

timothysc left a comment

duglin commented Nov 8, 2018

k8s-ci-robot commented Nov 9, 2018

duglin commented Nov 9, 2018

duglin commented Nov 9, 2018

duglin commented Nov 9, 2018

Some minor fixes for warnings while running the tests #68481

Some minor fixes for warnings while running the tests #68481

Conversation

duglin commented Sep 10, 2018

k8s-ci-robot commented Sep 10, 2018

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

linyouchong Sep 11, 2018 • 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

duglin commented Sep 10, 2018

duglin commented Sep 10, 2018

neolit123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

islinwb commented Sep 11, 2018

duglin commented Sep 11, 2018

neolit123 commented Sep 11, 2018

duglin commented Sep 11, 2018

duglin commented Sep 11, 2018

neolit123 commented Sep 11, 2018

duglin commented Sep 11, 2018

duglin commented Sep 25, 2018

neolit123 commented Sep 25, 2018

neolit123 commented Sep 28, 2018

timothysc left a comment

Choose a reason for hiding this comment

duglin commented Nov 4, 2018

neolit123 commented Nov 4, 2018

andyzhangx left a comment

Choose a reason for hiding this comment

timothysc left a comment

Choose a reason for hiding this comment

duglin commented Nov 8, 2018

k8s-ci-robot commented Nov 9, 2018

duglin commented Nov 9, 2018

duglin commented Nov 9, 2018

duglin commented Nov 9, 2018

linyouchong Sep 11, 2018 •

edited