Skip to content

Conversation

@grokspawn
Copy link
Contributor

Description of the change:
Capture JSON unmarshal type violations in greater detail to help authors spot the issue.

for e.g., when we set a properties type entry as type 4 instead of a string, we get a more descriptive blob to help us spot the violation:

./bin/opm render -o yaml /tmp/community-operator-index-4.12
2022/11/14 14:49:08 render reference "/tmp/community-operator-index-4.12": unmarshal error: json: cannot unmarshal number into Go struct field Property.properties.type of type string at offset 230 (indicated by <==)
 {"image":"quay.io/openshift-community-operators/zookeeper-operator@sha256:c55e04596b97a1571675d59a7594e6c28f819c5ba1b2907561219ef334f50002","name":"zookeeper-operator.v0.10.3","package":"zookeeper-operator","properties":[{"type":4 <==

Solves #1039

Motivation for the change:

Reviewer Checklist

  • Implementation matches the proposed design, or proposal is updated to match implementation
  • Sufficient unit test coverage
  • Sufficient end-to-end test coverage
  • Docs updated or added to /docs
  • Commit messages sensible and descriptive

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 14, 2022
@grokspawn grokspawn force-pushed the unmarshall-error-details branch from 53a9b48 to 1636b05 Compare November 14, 2022 21:06
if e, ok := err.(*json.UnmarshalFieldError); ok {
return e.Error()
}
if e, ok := err.(*json.InvalidUnmarshalError); ok {
return e.Error()
}
if e, ok := err.(*json.SyntaxError); ok {
return e.Error()
}
return err.Error()
Copy link
Member

Choose a reason for hiding this comment

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

All the error type assertions that end up returning .Error() could be condensed down to return err.Error(), right?

(Also for future reference, it's typically preferential to use errors.As in cases like this, because it means you can still get to the error you're after even if some code in between you and the error site has wrapped 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.

This was reviewer-bait. I was hoping that someone would be opinionated about some of these other error types. Otherwise, you're right ... they're just a series of "why didn't you just do the single fallthrough for all them" instead? 😄

Copy link
Contributor

@everettraven everettraven Nov 15, 2022

Choose a reason for hiding this comment

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

Taking a look at the additional different error types the only one I would probably do anything specific with is the json.SyntaxError type. I can see that this is another potential error that a user may want a more descriptive error for.

The json.InvalidUnmarshalError seems more related to an error from incorrect parameters passed to the json.Unmarshal function and the json.UnmarshalFieldError is now deprecated and no longer used so we don't need to do anything unique here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the reviews, guys!
I updated the code to support both of the errors we're initially interested in, and also reformatted the output to make it more human-readable.
The error scenario reflected in the PR description now displays as:

./bin/opm render -o yaml /tmp/community-operator-index-4.12
2022/11/15 16:00:38 render reference "/tmp/community-operator-index-4.12": unmarshal error: json: cannot unmarshal number into Go struct field Property.properties.type of type string at offset 230 (indicated by <==)
 {
    "image": "quay.io/openshift-community-operators/zookeeper-operator@sha256:c55e04596b97a1571675d59a7594e6c28f819c5ba1b2907561219ef334f50002",
    "name": "zookeeper-operator.v0.10.3",
    "package": "zookeeper-operator",
    "properties": [
        {
            "type": 4 <== ,
            "value": {
                "group": "zookeeper.streamnative.io",
                "kind": "ZooKeeperCluster",
                "version": "v1alpha1"
            }
        },
        {
            "type": "olm.package",
            "value": {
                "packageName": "zookeeper-operator",
                "version": "0.10.3"
            }
        }
    ],
    "relatedImages": [
        {
            "image": "docker.cloudsmith.io/streamnative/operators/zookeeper-operator:v0.10.3",
            "name": ""
        },
        {
            "image": "gcr.io/kubebuilder/kube-rbac-proxy:v0.5.0",
            "name": ""
        },
        {
            "image": "quay.io/openshift-community-operators/zookeeper-operator@sha256:c55e04596b97a1571675d59a7594e6c28f819c5ba1b2907561219ef334f50002",
            "name": ""
        }
    ],
    "schema": "olm.bundle"
}

@codecov
Copy link

codecov bot commented Nov 15, 2022

Codecov Report

Merging #1040 (6488837) into master (8959418) will increase coverage by 0.05%.
The diff coverage is 70.37%.

@@            Coverage Diff             @@
##           master    #1040      +/-   ##
==========================================
+ Coverage   51.95%   52.01%   +0.05%     
==========================================
  Files         102      102              
  Lines        9215     9241      +26     
==========================================
+ Hits         4788     4807      +19     
- Misses       3514     3521       +7     
  Partials      913      913              
Impacted Files Coverage Δ
alpha/declcfg/load.go 76.52% <70.37%> (-2.13%) ⬇️
alpha/veneer/semver/semver.go 60.50% <0.00%> (+0.50%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Contributor

@everettraven everettraven left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 16, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: everettraven, grokspawn

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:

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

Signed-off-by: Jordan Keister <jordan@nimblewidget.com>
Signed-off-by: Jordan Keister <jordan@nimblewidget.com>
@grokspawn grokspawn force-pushed the unmarshall-error-details branch from df934e9 to 6488837 Compare November 16, 2022 18:57
@oceanc80
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 30, 2022
@openshift-merge-robot openshift-merge-robot merged commit 249ae62 into operator-framework:master Nov 30, 2022
@grokspawn grokspawn deleted the unmarshall-error-details branch December 1, 2022 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants