Skip to content
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.

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

Fix JSON protection unmarshal error #2606

Merged
merged 1 commit into from
Dec 26, 2022
Merged

Conversation

xorima
Copy link
Contributor

@xorima xorima commented Dec 23, 2022

Description of change

Currently when trying to load in protection an error is thrown due to a data structure mismatch resulting in errors:

json: cannot unmarshal object into Go struct field Protection.lock_branch of type bool 
json: cannot unmarshal object into Go struct field Protection.allow_fork_syncing of type bool

This is due to them using an enabled field which the current structure does not understand. This change introduces the relevant structure and has been tested locally.

API Payload:

{
    "url": "https://api.github.com/repos/sous-chefs/sc_vscode/branches/main/protection",
   ...
    "required_conversation_resolution": {
        "enabled": false
    },
    "lock_branch": {
        "enabled": false
    },
    "allow_fork_syncing": {
        "enabled": false
    }
}

As can be seen on the payload above they are under the enabled key and are not bool fields directly in their own right.

Signed-off-by: Jason Field [email protected]

Currently when trying to load in protection an error is thrown due to
a data structure mismatch resulting in errors:
json: cannot unmarshal object into Go struct field Protection.lock_branch of type bool
json: cannot unmarshal object into Go struct field Protection.allow_fork_syncing of type bool
This is due to them using an enabled field which the current structure
does not understand. This change introduces the relevant structure and
has been tested locally.

Signed-off-by: Jason Field <[email protected]>
@gmlewis gmlewis changed the title fix(protection): Resolves json unmarshal protection error Fix JSON protection unmarshal error Dec 23, 2022
@gmlewis gmlewis added the Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). label Dec 23, 2022
@codecov
Copy link

codecov bot commented Dec 23, 2022

Codecov Report

Merging #2606 (2455d5d) into master (2b4d596) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2606   +/-   ##
=======================================
  Coverage   97.99%   97.99%           
=======================================
  Files         126      126           
  Lines       10913    10913           
=======================================
  Hits        10694    10694           
  Misses        150      150           
  Partials       69       69           
Impacted Files Coverage Δ
github/repos.go 98.67% <ø> (ø)

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

Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thank you, @xorima !
LGTM.

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

@gmlewis gmlewis added the bug label Dec 23, 2022
Copy link
Contributor

@valbeat valbeat left a comment

Choose a reason for hiding this comment

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

LGTM!

@gmlewis
Copy link
Collaborator

gmlewis commented Dec 26, 2022

Thank you, @valbeat !
Merging.

@gmlewis gmlewis merged commit c5d656a into google:master Dec 26, 2022
@xorima xorima deleted the fix/Protection branch December 26, 2022 13:51
@xorima
Copy link
Contributor Author

xorima commented Jan 2, 2023

Thanks @gmlewis, can we get a release cut in the next week or two so I don't have to continue to use my fork?

@gmlewis
Copy link
Collaborator

gmlewis commented Jan 3, 2023

@xorima - might you have time to review #2611 (so that we can incorporate it into the next release, as I think that will be an important one to include)?

@xorima
Copy link
Contributor Author

xorima commented Jan 3, 2023

Sure I'll try to take a look in the morning @gmlewis

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants