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

Support NTP server details in base, EOS #2160

Merged
merged 7 commits into from
Feb 10, 2025
Merged

Support NTP server details in base, EOS #2160

merged 7 commits into from
Feb 10, 2025

Conversation

bewing
Copy link
Member

@bewing bewing commented Jan 22, 2025

Based on models from openconfig-system, return optional additional data regarding NTP servers for the first supported driver (EOS).

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)
Copy link
Contributor

@ktbyers ktbyers left a 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.

bewing and others added 4 commits January 23, 2025 09:09
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.
Copy link
Contributor

@ktbyers ktbyers left a 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,
Copy link
Contributor

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?

Copy link
Member Author

@bewing bewing Feb 10, 2025

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.

@bewing bewing merged commit 6cacce8 into develop Feb 10, 2025
12 checks passed
@bewing bewing deleted the ntp_details branch February 10, 2025 21:08
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.

2 participants