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

Added UnmarshalOpt to allow a best effort unmarshal #863

Merged
merged 17 commits into from
Jun 26, 2023

Conversation

lgomez9
Copy link
Contributor

@lgomez9 lgomez9 commented Jun 7, 2023

Added an option to allow a failing unmarshal will now pass - instead of failing, it will simply log the errors and continue the unmarshaling process. This is so that we can generate diffs for the entirety of the SetRequest, even if there's a compliance error.

…hal still will not pass, but now the unmarshal will continue, logging any errors it encounters instead of exiting immediately.
ytypes/unmarshal.go Outdated Show resolved Hide resolved
ytypes/unmarshal.go Outdated Show resolved Hide resolved
ytypes/unmarshal.go Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Jun 8, 2023

Coverage Status

coverage: 89.664%. remained the same when pulling cd4f0bc on lgomez9:best_effort_unmarshal into a2af562 on openconfig:master.

ytypes/unmarshal.go Outdated Show resolved Hide resolved
ytypes/gnmi.go Outdated Show resolved Hide resolved
ytypes/gnmi.go Outdated Show resolved Hide resolved
ytypes/gnmi.go Outdated Show resolved Hide resolved
@lgomez9 lgomez9 requested a review from wenovus June 15, 2023 15:22
ytypes/unmarshal.go Outdated Show resolved Hide resolved
ytypes/unmarshal.go Outdated Show resolved Hide resolved
@lgomez9 lgomez9 requested a review from wenovus June 20, 2023 12:35
Copy link
Collaborator

@wenovus wenovus left a comment

Choose a reason for hiding this comment

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

PTAL at the presubmit failures.

ytypes/unmarshal.go Outdated Show resolved Hide resolved
ytypes/gnmi_test.go Outdated Show resolved Hide resolved
@lgomez9 lgomez9 requested a review from wenovus June 21, 2023 20:40
ytypes/gnmi_test.go Outdated Show resolved Hide resolved
ytypes/gnmi_test.go Outdated Show resolved Hide resolved
@lgomez9 lgomez9 requested a review from wenovus June 22, 2023 17:17
Copy link
Collaborator

@wenovus wenovus left a comment

Choose a reason for hiding this comment

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

Thanks!

@lgomez9 lgomez9 requested a review from wenovus June 22, 2023 18:55
Copy link
Collaborator

@wenovus wenovus left a comment

Choose a reason for hiding this comment

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

Missed reviewing the coverage, can you add coverage for one part of the code and the rest looks good.

ytypes/unmarshal.go Show resolved Hide resolved
@lgomez9 lgomez9 requested a review from wenovus June 26, 2023 15:38
Copy link
Collaborator

@wenovus wenovus 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, LGTM

@lgomez9 lgomez9 requested a review from wenovus June 26, 2023 16:21
@lgomez9
Copy link
Contributor Author

lgomez9 commented Jun 26, 2023

Sorry, thought I had run gofmt, but forgot to.

Copy link
Collaborator

@wenovus wenovus left a comment

Choose a reason for hiding this comment

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

Thanks! merged

@wenovus wenovus merged commit efad7f9 into openconfig:master Jun 26, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants