-
Notifications
You must be signed in to change notification settings - Fork 561
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
Support NTP server details in base, EOS #2160
Conversation
In support of adding details to NTP server dictionaries, support detection of non-total TypedDicts and change the logic to only error if there are EXTRA keys instead of missing keys.
Support returning details regarding individual NTP servers based on openconfig models (/system/ntp/servers/server)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only reviewed part of the testing here...but it is a start.
Since we only have a limited set of models that we are testing subsets of keys for, we should explicitly only allow subsets when testing those getters.
Making NTPPeerDict and NTPServerDict non-identical caused issues with typing since we wre unable to assert the type of the return dictionary easily. This refactor allows us to explicitly state the return type for each function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
"prefer": bool, | ||
"network_instance": str, | ||
"source_address": str, | ||
"key_id": int, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So these are all optional key-values which may be returned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is correct. If we get to the point where all drivers return it in all situations, we could make it non-optional.
Based on models from openconfig-system, return optional additional data regarding NTP servers for the first supported driver (EOS).