Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 24, 2023
+44 −15
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .golangci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ linters:
- dupword
- durationcheck
- errcheck
# - errchkjson
- errchkjson
- exportloopref
- gci
# - ginkgolinter
Expand Down
5 changes: 4 additions & 1 deletion packaging/flavorgen/flavors/flavors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,10 @@ func ClusterClassTemplateWithKubeVIP() []runtime.Object {
}

func ClusterTopologyTemplateKubeVIP() []runtime.Object {
cluster := newClusterClassCluster()
cluster, err := newClusterTopologyCluster()
if err != nil {
return nil
Copy link
Member

@sbueringer sbueringer Aug 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, totally missed that 🤦

Copy link
Member

@sbueringer sbueringer Aug 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah looks like Christian opened a PR already

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, all good

}
identitySecret := newIdentitySecret()
clusterResourceSet := newClusterResourceSet(cluster)
crsResourcesCSI := crs.CreateCrsResourceObjectsCSI(&clusterResourceSet)
Expand Down
52 changes: 39 additions & 13 deletions packaging/flavorgen/flavors/generators.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"encoding/json"
"fmt"

"github.com/pkg/errors"
corev1 "k8s.io/api/core/v1"
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -94,7 +95,11 @@ systemd:
EnvironmentFile=/run/metadata/*`
)

func newClusterClassCluster() clusterv1.Cluster {
func newClusterTopologyCluster() (clusterv1.Cluster, error) {
variables, err := clusterTopologyVariables()
if err != nil {
return clusterv1.Cluster{}, errors.Wrap(err, "failed to create ClusterTopologyCluster template")
}
return clusterv1.Cluster{
TypeMeta: metav1.TypeMeta{
APIVersion: clusterv1.GroupVersion.String(),
Expand All @@ -121,17 +126,34 @@ func newClusterClassCluster() clusterv1.Cluster {
},
},
},
Variables: clusterTopologyVariables(),
Variables: variables,
},
},
}
}, nil
}

func clusterTopologyVariables() []clusterv1.ClusterVariable {
sshKey, _ := json.Marshal(env.VSphereSSHAuthorizedKeysVar)
controlPlaneIP, _ := json.Marshal(env.ControlPlaneEndpointVar)
secretName, _ := json.Marshal(env.ClusterNameVar)
kubeVipPod, _ := json.Marshal(kubeVIPPodYaml())
func clusterTopologyVariables() ([]clusterv1.ClusterVariable, error) {
sshKey, err := json.Marshal(env.VSphereSSHAuthorizedKeysVar)
if err != nil {
return nil, errors.Wrapf(err, "failed to json-encode variable VSphereSSHAuthorizedKeysVar: %q", env.VSphereSSHAuthorizedKeysVar)
}
controlPlaneIP, err := json.Marshal(env.ControlPlaneEndpointVar)
if err != nil {
return nil, errors.Wrapf(err, "failed to json-encode variable ControlPlaneEndpointVar: %q", env.ControlPlaneEndpointVar)
}
secretName, err := json.Marshal(env.ClusterNameVar)
if err != nil {
return nil, errors.Wrapf(err, "failed to json-encode variable ClusterNameVar: %q", env.ClusterNameVar)
}
kubeVipPodYaml := kubeVIPPodYaml()
kubeVipPod, err := json.Marshal(kubeVipPodYaml)
if err != nil {
return nil, errors.Wrapf(err, "failed to json-encode variable kubeVipPod: %q", kubeVipPodYaml)
}
infraServerValue, err := getInfraServerValue()
if err != nil {
return nil, err
}
return []clusterv1.ClusterVariable{
{
Name: "sshKey",
Expand All @@ -142,7 +164,7 @@ func clusterTopologyVariables() []clusterv1.ClusterVariable {
{
Name: "infraServer",
Value: apiextensionsv1.JSON{
Raw: getInfraServerValue(),
Raw: infraServerValue,
},
},
{
Expand All @@ -164,15 +186,19 @@ func clusterTopologyVariables() []clusterv1.ClusterVariable {
Raw: secretName,
},
},
}
}, nil
}

func getInfraServerValue() []byte {
byteArr, _ := json.Marshal(map[string]string{
func getInfraServerValue() ([]byte, error) {
byteArr, err := json.Marshal(map[string]string{
"url": env.VSphereServerVar,
"thumbprint": env.VSphereThumbprint,
})
return byteArr
if err != nil {
return nil, errors.Wrapf(err, "failed to json-encode, VSphereServerVar: %s, VSphereThumbprint: %s",
env.VSphereServerVar, env.VSphereThumbprint)
}
return byteArr, nil
}

func newVSphereCluster() infrav1.VSphereCluster {
Expand Down