upup/pkg/fi-fix staticcheck #8193

Aresforchina · 2019-12-24T03:54:46Z

ref:#7800
special notes:
upup/pkg/fi/cloudup/apply_cluster.go:300:15: printf-style function with dynamic format string and no further arguments should use print-style function instead (SA1006)
upup/pkg/fi/cloudup/apply_cluster.go:307:15: printf-style function with dynamic format string and no further arguments should use print-style function instead (SA1006)
upup/pkg/fi/cloudup/apply_cluster.go:1003:14: printf-style function with dynamic format string and no further arguments should use print-style function instead (SA1006)
upup/pkg/fi/cloudup/apply_cluster.go:1010:14: printf-style function with dynamic format string and no further arguments should use print-style function instead (SA1006)
upup/pkg/fi/cloudup/apply_cluster.go:1014:14: printf-style function with dynamic format string and no further arguments should use print-style function instead (SA1006)
upup/pkg/fi/cloudup/apply_cluster.go:1025:14: printf-style function with dynamic format string and no further arguments should use print-style function instead (SA1006)
upup/pkg/fi/cloudup/apply_cluster.go:1069:14: printf-style function with dynamic format string and no further arguments should use print-style function instead (SA1006)
upup/pkg/fi/cloudup/apply_cluster.go:1076:14: printf-style function with dynamic format string and no further arguments should use print-style function instead (SA1006)
upup/pkg/fi/cloudup/apply_cluster.go:1080:14: printf-style function with dynamic format string and no further arguments should use print-style function instead (SA1006)
upup/pkg/fi/cloudup/apply_cluster.go:1091:14: printf-style function with dynamic format string and no further arguments should use print-style function instead (SA1006)
upup/pkg/fi/cloudup/awstasks/sshkey.go:206:79: argument a is overwritten before first use (SA4009)
upup/pkg/fi/cloudup/loader.go:58:2: field templates is unused (U1000)
upup/pkg/fi/executor.go:162:6: type runnable is unused (U1000)

k8s-ci-robot · 2019-12-24T03:54:54Z

Hi @Aresforchina. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

Aresforchina · 2019-12-24T03:57:06Z

/assign @chrisz100

tanjunchen · 2019-12-24T06:20:22Z

/ok-to-test

Aresforchina · 2019-12-24T08:10:55Z

/test pull-kops-e2e-kubernetes-aws

rifelpet · 2019-12-24T15:14:54Z

upup/pkg/fi/cloudup/awstasks/sshkey.go

@@ -208,7 +208,7 @@ func (_ *SSHKey) RenderCloudformation(t *cloudformation.CloudformationTarget, a,

 	klog.Warningf("Cloudformation does not manage SSH keys; pre-creating SSH key")

-	a, err := e.find(cloud)
+	_, err := e.find(cloud)


this a variable is used by line 216, correct? I don't think replacing it with _ is the correct fix here.

this instance shouldn't get flagged – because it is only being set conditionally, in one branch.or can you give me some advice,please?

The mechanical fix would be to change the parameter a to _. But then, why is there a parameter? Is the e.find(cloud) call needed?

This function is called using reflection, so changing its signature may not be feasible:

kops/upup/pkg/fi/context.go

Lines 218 to 223 in 5cb2d36

rendererArgs = append(rendererArgs, reflect.ValueOf(a))

rendererArgs = append(rendererArgs, reflect.ValueOf(e))

rendererArgs = append(rendererArgs, reflect.ValueOf(changes))

klog.V(11).Infof("Calling method %s on %T", renderer.Name, e)

m := v.MethodByName(renderer.Name)

rv := m.Call(rendererArgs)

Renaming the parameter to _ might be the best approach. I'm sure there are other tasks that have unused a parameters in some of their Render functions, perhaps we look at how it is handled elsewhere?

My question was more along the lines of "Is the value of the a parameter semantically different than the a obtained from e.find(cloud)? If not, the call wouldn't be needed.

After reviewing the commit that added these lines, I understand the context a bit more. For reference, a is the "actual" state of the task, e is the "expected" state of the task.

For CloudFormation (and Terraform) targets we dont care about the actual state of the resource in AWS, we're generating the same cloudformation/terraform output regardless. Therefor most tasks' RenderTerraform functions don't use their a parameters. SSHKey is an exception because CloudFormation doesnt support creating them, so Kops needs to create the key AWS directly, which requires knowing the actual state of the ssh key. Now the question becomes "does the a parameter get populated with the true actual state of the resource even for cloudformation targets?" ~~and based on some testing I believe the answer is yes. Therefor I think we can get rid of this line and the error checking directly below it.~~

Sorry I misspoke, a does not get populated with cloudformation target, therefor the best fix would be to do what other tasks do and use a different variable name, perhaps keypair.

Aresforchina · 2019-12-30T08:45:09Z

/test pull-kops-verify-staticcheck

tanjunchen · 2019-12-31T07:12:53Z

@Aresforchina
can you fix the conflicts , thanks

Aresforchina · 2019-12-31T07:20:43Z

@tanjunchen yes,I will fix them,thanks!

tanjunchen · 2020-01-02T02:49:44Z

/test pull-kops-verify-staticcheck

hakman · 2020-01-02T10:45:22Z

upup/pkg/fi/cloudup/apply_cluster.go

 		fmt.Printf("\n")
 		fmt.Printf("A new kubernetes version is available: %s\n", recommended)
 		fmt.Printf("Upgrading is recommended (try kops upgrade cluster)\n")
 		fmt.Printf("\n")
 		fmt.Printf("More information: %s\n", buildPermalink("upgrade_k8s", recommended.String()))
 		fmt.Printf("\n")
-		fmt.Printf(starline)
+		fmt.Print(starline)


To keep the same style, same in a few other places:

Suggested change

fmt.Print(starline)

fmt.Printf("%s\n"), starline)

yes,sir.I have fixed them together.

hakman

I think there are a bit too many changes in apply_cluster.go now. I suggest to check this version.
https://gist.githubusercontent.com/hakman/b980c8140305393e475c96952fca6d7e/raw/1fe983c96b6750f2091ef8d06974cf512830bd91/apply_cluster.go

Aresforchina · 2020-01-07T02:15:40Z

@hakman you give me this version file that print style as println() ,but last time suggest me update it into fmt.Printf("%s\n"), starline). so could I entirely change print style into println() in apply_cluster.go now?

johngmyers · 2020-01-07T05:10:53Z

upup/pkg/fi/cloudup/apply_cluster.go

@@ -299,14 +299,14 @@ func (c *ApplyClusterCmd) Run() error {

 		if warn {
 			fmt.Println("")
-			fmt.Println(starline)
+			fmt.Printf(("%s\n"), starline)


Why all the extra parentheses?

This is my syntax fault .Thank you for pointing it out.

hakman · 2020-01-07T05:16:53Z

@Aresforchina sorry for the confusion. I wanted to suggest to change that in areas where there was mostly printf.

As @johngmyers pointed out, there were also some extra parenthesis and new lines in the current version. Seemed easier to just provide a full file as suggestion.

Aresforchina · 2020-01-07T06:43:08Z

@hakman Thanks for your patience. I will fix it as soon as possible.

Aresforchina · 2020-01-07T08:35:25Z

/test pull-kops-e2e-kubernetes-aws

Aresforchina · 2020-01-10T07:44:53Z

/cc @rifelpet can you give me advice in this pr,please?

johngmyers · 2020-01-10T18:06:53Z

/retest

justinsb · 2020-01-11T19:41:02Z

I think this looks good - thanks for sticking with it @Aresforchina

/approve
/lgtm

k8s-ci-robot · 2020-01-11T19:41:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Aresforchina, justinsb

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:

~~OWNERS~~ [justinsb]

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

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 24, 2019

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 24, 2019

k8s-ci-robot requested review from joshbranham and robinpercy December 24, 2019 03:55

k8s-ci-robot assigned chrisz100 Dec 24, 2019

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 24, 2019

rifelpet reviewed Dec 24, 2019

View reviewed changes

Aresforchina force-pushed the fix-staticheckout-pkg-resource02 branch from 1a837c8 to 567dc77 Compare December 30, 2019 08:31

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 31, 2019

hakman mentioned this pull request Dec 31, 2019

upup/pkg/fi/cloudup/awstasks/ staticcheck #8235

Merged

Aresforchina force-pushed the fix-staticheckout-pkg-resource02 branch 2 times, most recently from ce1a3ad to 528f422 Compare January 2, 2020 06:47

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 2, 2020

hakman reviewed Jan 2, 2020

View reviewed changes

Aresforchina force-pushed the fix-staticheckout-pkg-resource02 branch 2 times, most recently from 4ffb3a7 to 6ef2e19 Compare January 6, 2020 04:34

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 6, 2020

Aresforchina force-pushed the fix-staticheckout-pkg-resource02 branch from 6ef2e19 to e870727 Compare January 6, 2020 05:58

hakman requested changes Jan 6, 2020

View reviewed changes

johngmyers reviewed Jan 7, 2020

View reviewed changes

Aresforchina force-pushed the fix-staticheckout-pkg-resource02 branch from e870727 to 87904db Compare January 7, 2020 06:57

upup/pkg/fi-fix staticcheck

f312f7c

Aresforchina force-pushed the fix-staticheckout-pkg-resource02 branch from 87904db to f312f7c Compare January 7, 2020 07:51

k8s-ci-robot assigned justinsb Jan 11, 2020

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 11, 2020

k8s-ci-robot merged commit 5561759 into kubernetes:master Jan 11, 2020

k8s-ci-robot added this to the v1.18 milestone Jan 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

upup/pkg/fi-fix staticcheck #8193

upup/pkg/fi-fix staticcheck #8193

Aresforchina commented Dec 24, 2019

k8s-ci-robot commented Dec 24, 2019

Aresforchina commented Dec 24, 2019

tanjunchen commented Dec 24, 2019

Aresforchina commented Dec 24, 2019

rifelpet Dec 24, 2019

Aresforchina Dec 25, 2019

johngmyers Dec 25, 2019

rifelpet Dec 25, 2019 •

edited

Loading

johngmyers Dec 25, 2019

rifelpet Dec 26, 2019 •

edited

Loading

rifelpet Dec 26, 2019

Aresforchina commented Dec 30, 2019

tanjunchen commented Dec 31, 2019

Aresforchina commented Dec 31, 2019

tanjunchen commented Jan 2, 2020

hakman Jan 2, 2020

Aresforchina Jan 3, 2020

hakman left a comment

Aresforchina commented Jan 7, 2020 •

edited

Loading

johngmyers Jan 7, 2020

Aresforchina Jan 7, 2020

hakman commented Jan 7, 2020

Aresforchina commented Jan 7, 2020

Aresforchina commented Jan 7, 2020

Aresforchina commented Jan 10, 2020

johngmyers commented Jan 10, 2020

justinsb commented Jan 11, 2020

k8s-ci-robot commented Jan 11, 2020

	rendererArgs = append(rendererArgs, reflect.ValueOf(a))
	rendererArgs = append(rendererArgs, reflect.ValueOf(e))
	rendererArgs = append(rendererArgs, reflect.ValueOf(changes))
	klog.V(11).Infof("Calling method %s on %T", renderer.Name, e)
	m := v.MethodByName(renderer.Name)
	rv := m.Call(rendererArgs)

upup/pkg/fi-fix staticcheck #8193

upup/pkg/fi-fix staticcheck #8193

Conversation

Aresforchina commented Dec 24, 2019

k8s-ci-robot commented Dec 24, 2019

Aresforchina commented Dec 24, 2019

tanjunchen commented Dec 24, 2019

Aresforchina commented Dec 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rifelpet Dec 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rifelpet Dec 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aresforchina commented Dec 30, 2019

tanjunchen commented Dec 31, 2019

Aresforchina commented Dec 31, 2019

tanjunchen commented Jan 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hakman left a comment

Choose a reason for hiding this comment

Aresforchina commented Jan 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hakman commented Jan 7, 2020

Aresforchina commented Jan 7, 2020

Aresforchina commented Jan 7, 2020

Aresforchina commented Jan 10, 2020

johngmyers commented Jan 10, 2020

justinsb commented Jan 11, 2020

k8s-ci-robot commented Jan 11, 2020

rifelpet Dec 25, 2019 •

edited

Loading

rifelpet Dec 26, 2019 •

edited

Loading

Aresforchina commented Jan 7, 2020 •

edited

Loading