🌱 lint: fix the JSON encoding check error via errchkjson linter #2273

MrDXY · 2023-08-23T04:17:27Z

What this PR does / why we need it:

Fix the checked error by errchkjson linter. (remove the safe returns of errors from json.Marshal)
Enable errchkjson check in .golangci.yml configuration file.
Enable linters-settings.errchkjson.check-error-free-encoding. With it set to true, errchkjson does not report about the errors from json encoding functions that are safe to be ignored, because they are not possible to happen.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Part of #2058

k8s-ci-robot · 2023-08-23T04:17:36Z

Hi @MrDXY. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

chrischdi · 2023-08-23T06:09:15Z

.golangci.yml

+  errchkjson:
+    check-error-free-encoding: true


Suggested change

errchkjson:

check-error-free-encoding: true

We should not set this to be equal to CAPI and resolve the findings instead by handling the error.

Sure, I can make the necessary adjustments here, and it totally makes sense that we prefer to keep consistent with CAPI.

To discuss it further:
This check-error-free-encoding parameter here, is just to indicate the errchkjson to ignore the errors that will never happen.

Especially in some cases:

sshKey, _ := json.Marshal(env.VSphereSSHAuthorizedKeysVar) controlPlaneIP, _ := json.Marshal(env.ControlPlaneEndpointVar) secretName, _ := json.Marshal(env.ClusterNameVar) kubeVipPod, _ := json.Marshal(kubeVIPPodYaml())

the input of the json.Marsha() is just a string, which will never cause the function to fail, IIRC. To handle such a situation, is it a good idea to propagate errors to the higher-level calling function? Or do you recall any recommended approaches that I could follow?

I did see that the required changes would only be in the flavorgen tool. So just bubbling them up should be fine.

Sorry to ask, just want to make sure, bubbing them up means bubbling the error up to the higher-level function, right?

Yes. No worries, just ask all good :)

Alright. So keeping it as is is fine :)

I'm fine with keeping it as it's the consensus - but I don't really like relying on external linters like this for something this fundamental, at the cost of saving a few lines of if err == nil'{}

Good point on relying on an external linter. Basically if they have a bug in their logic we inherit that in CAPI as well

Alright, changing my opinion again. Now I'm in favor of dropping the check-error-free-encoding config

@chrischdi It's totally fine! Still, welcome any advice or suggestions you may have.
@sbueringer Got it!

pkg/services/vmoperator/vmopmachine.go

killianmuldoon

/ok-to-test

sbueringer · 2023-08-23T16:50:27Z

/lgtm

/assign @killianmuldoon @chrischdi

~~Would be nice, if someone has time, to follow-up to do the same in core CAPI. I think it's really helpful if we get rid of error branches that can never happen.~~

See: #2273 (comment)

sbueringer · 2023-08-23T17:04:10Z

/lgtm cancel

killianmuldoon

/lgtm

Thanks for this!

k8s-ci-robot · 2023-08-24T07:54:10Z

LGTM label has been added.

Git tree hash: 4fc5dadd58eb94498c98ead0031f94e130941a92

chrischdi

/lgtm
/approve

Thanks!

k8s-ci-robot · 2023-08-24T08:02:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrischdi

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~~ [chrischdi]

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

sbueringer · 2023-08-24T08:46:49Z

packaging/flavorgen/flavors/flavors.go

-	cluster := newClusterClassCluster()
+	cluster, err := newClusterTopologyCluster()
+	if err != nil {
+		return nil


I think we should return the error here

I think if we choose to handle the error we should propagate (bubble) it up the entire call hierarchy

I have considered this previously, and I choose to return nil for the following reason:

the function ClusterTopologyTemplateKubeVIP() []runtime.Object does not have a return value for the error, so I'll need to modify the API structure to fit it in. While this method is public, I'm not sure if this will cause some trouble for other customers who might be using it to generate their templates.

Since we know that this method will not actually produce an error (the json-encoding error is safe to ignore), it may be okay not to expose it.

But it's also OK. If we consider it inappropriate to ignore the error. I can make some adjustments to the function.

Oh, totally missed that 🤦

Let's make the adjustment to the func to add the error return parameter. I think for this sort of code it should be okay if we evolve it a bit over time (not sure if anyone apart from us is using this util / it's exported functions)

It just feels consistent to handle the error if we choose to handle it and not drop it one level up the call hierarchy :)

Ah looks like Christian opened a PR already

I created #2274 so we don't forget to fix that.

In this case its way better to bubble up even if we currently know the current code does not produce an error.

There should either only be the two extremes:

bubbling up

or ignore the error where we get it, not somewhere in-between

Yup, all good

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 23, 2023

k8s-ci-robot requested review from chrischdi and zhanggbj August 23, 2023 04:17

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 23, 2023

MrDXY mentioned this pull request Aug 23, 2023

Audit golangci-lint config #2058

Closed

chrischdi requested changes Aug 23, 2023

View reviewed changes

sbueringer reviewed Aug 23, 2023

View reviewed changes

pkg/services/vmoperator/vmopmachine.go Outdated Show resolved Hide resolved

killianmuldoon reviewed Aug 23, 2023

View reviewed changes

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 Aug 23, 2023

k8s-ci-robot assigned chrischdi, killianmuldoon and sbueringer Aug 23, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 23, 2023

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 23, 2023

k8s-ci-robot requested review from chrischdi, killianmuldoon and sbueringer August 23, 2023 17:04

MrDXY force-pushed the pr-golangci-sync-errchkjson branch from ec6c555 to b23ef2d Compare August 24, 2023 03:45

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 24, 2023

MrDXY force-pushed the pr-golangci-sync-errchkjson branch from b23ef2d to 27c14a9 Compare August 24, 2023 03:48

lint: fix checks for the json encoding function.

2e2f203

MrDXY force-pushed the pr-golangci-sync-errchkjson branch from 27c14a9 to 2e2f203 Compare August 24, 2023 04:20

killianmuldoon reviewed Aug 24, 2023

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 24, 2023

chrischdi reviewed Aug 24, 2023

View reviewed changes

chrischdi approved these changes Aug 24, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 24, 2023

k8s-ci-robot merged commit 2ddce8c into kubernetes-sigs:main Aug 24, 2023
7 checks passed

k8s-ci-robot added this to the v1.9 milestone Aug 24, 2023

sbueringer reviewed Aug 24, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 lint: fix the JSON encoding check error via errchkjson linter #2273

🌱 lint: fix the JSON encoding check error via errchkjson linter #2273

MrDXY commented Aug 23, 2023 •

edited

Loading

k8s-ci-robot commented Aug 23, 2023

chrischdi Aug 23, 2023

MrDXY Aug 23, 2023

chrischdi Aug 23, 2023

MrDXY Aug 23, 2023 •

edited

Loading

sbueringer Aug 23, 2023

sbueringer Aug 23, 2023

killianmuldoon Aug 23, 2023

sbueringer Aug 23, 2023

sbueringer Aug 23, 2023

MrDXY Aug 24, 2023

killianmuldoon left a comment

sbueringer commented Aug 23, 2023 •

edited

Loading

sbueringer commented Aug 23, 2023

killianmuldoon left a comment

k8s-ci-robot commented Aug 24, 2023

chrischdi left a comment

k8s-ci-robot commented Aug 24, 2023

sbueringer Aug 24, 2023 •

edited

Loading

MrDXY Aug 24, 2023

MrDXY Aug 24, 2023

chrischdi Aug 24, 2023

sbueringer Aug 24, 2023 •

edited

Loading

sbueringer Aug 24, 2023

chrischdi Aug 24, 2023

sbueringer Aug 24, 2023

🌱 lint: fix the JSON encoding check error via errchkjson linter #2273

🌱 lint: fix the JSON encoding check error via errchkjson linter #2273

Conversation

MrDXY commented Aug 23, 2023 • edited Loading

k8s-ci-robot commented Aug 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrDXY Aug 23, 2023 • edited Loading

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

killianmuldoon left a comment

Choose a reason for hiding this comment

sbueringer commented Aug 23, 2023 • edited Loading

sbueringer commented Aug 23, 2023

killianmuldoon left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 24, 2023

chrischdi left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 24, 2023

sbueringer Aug 24, 2023 • edited Loading

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

sbueringer Aug 24, 2023 • edited Loading

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

MrDXY commented Aug 23, 2023 •

edited

Loading

MrDXY Aug 23, 2023 •

edited

Loading

sbueringer commented Aug 23, 2023 •

edited

Loading

sbueringer Aug 24, 2023 •

edited

Loading

sbueringer Aug 24, 2023 •

edited

Loading