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

Changing type for NTP fields in openconfig-system.yang #995

Closed
wants to merge 2 commits into from

Conversation

kbchandradeep
Copy link

Changing type for root-delay, root-dispersion and offset
from uint32, uint64 and uint64 respectively to oc-types:ieeefloat32

This difference in type has resulted in implementations to deviate these fields as not-supported.

root-delay, root-dispersion and offset
from uint32, uint64 and uint64 respectively to oc-types:ieeefloat32
@kbchandradeep kbchandradeep requested a review from a team as a code owner November 8, 2023 14:59
Copy link

google-cla bot commented Nov 8, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@OpenConfigBot
Copy link

OpenConfigBot commented Nov 8, 2023

No major YANG version changes in commit 4a84d0f

@jsterne
Copy link

jsterne commented Jan 11, 2024

One downside of ieeefloat32 type is that the underlying YANG type is binary. The native output of this in many clients will likely just be a raw byte dump (maybe in hex) and it will be pretty non-intuitive for humans looking at the values (unless the various clients or output functions add specific support for this ieeefloat32 type). Maybe that's not a top priority here but something to consider.

@dplore
Copy link
Member

dplore commented Jul 16, 2024

Is there some operational use case that cannot be met using the int values in the model? The model does not need to match the wire format. The model should be easy to consume, and converting between int and float is trivial to do in code.

There needs to be a strong operational use case to justify making this breaking change. I'll close this for now. You may reopen if you have some strong operational use case.

@dplore dplore closed this Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants