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

add minpoll&maxpoll for NTP server in sonic-ntp.yang #16310

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

bsun-sudo
Copy link
Contributor

@bsun-sudo bsun-sudo commented Aug 28, 2023

Why I did it

Add minpoll and maxpoll for NTP server in sonic-ntp.yang
PR for NTP HLD update is sonic-net/SONiC#1478

How I did it

Add minpoll and maxpoll for NTP server in sonic-ntp.yang. Added additional test cases in ntp.json and sample_config_db.json

How to verify it

Successfully built
sonic_yang_mgmt-1.0-py3-none-any.whl
sonic_yang_models-1.0-py3-none-any.whl

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 28, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@qiluo-msft qiluo-msft added the YANG YANG model related changes label Aug 28, 2023
@qiluo-msft qiluo-msft added this to To be reviewed in yang via automation Aug 28, 2023
@ganglyu ganglyu requested a review from wen587 August 28, 2023 23:27
@ganglyu
Copy link
Contributor

ganglyu commented Aug 28, 2023

@wen587
Would you please check the GCU error?

@wen587
Copy link
Contributor

wen587 commented Aug 29, 2023

Hi, the issue looks the same as YANG backlink issue.
The different part is that we can temp fix the previous backlink issue by commnet out sonic-yang include. This one is not caused by sonic-vlan include.

It is missing VLAN_MEMBER link after the YANG modification.

def test_find_ref_paths__path_is_table__returns_ref_paths(self):
# Arrange
path = "/PORT"
expected = [
"/ACL_TABLE/DATAACL/ports/0",
"/ACL_TABLE/EVERFLOW/ports/0",
"/ACL_TABLE/EVERFLOWV6/ports/0",
"/ACL_TABLE/EVERFLOWV6/ports/1",
"/ACL_TABLE/NO-NSW-PACL-V4/ports/0",
"/VLAN_MEMBER/Vlan1000|Ethernet0",
"/VLAN_MEMBER/Vlan1000|Ethernet4",
"/VLAN_MEMBER/Vlan1000|Ethernet8"

]

E First list contains 3 additional elements.
E First extra element 5:
E '/VLAN_MEMBER/Vlan1000|Ethernet0'
E
E ['/ACL_TABLE/DATAACL/ports/0',
E '/ACL_TABLE/EVERFLOW/ports/0',
E '/ACL_TABLE/EVERFLOWV6/ports/0',
E '/ACL_TABLE/EVERFLOWV6/ports/1',
E + '/ACL_TABLE/NO-NSW-PACL-V4/ports/0']

@wen587
Copy link
Contributor

wen587 commented Aug 29, 2023

It seems caused by another PR which involve the vlan import. I created revert PR for this issue: #16322

"NTP_SERVER_LIST": [
{
"server_address": "10.11.12.13",
"maxpoll": "5"
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the difference between this test case and the one above?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, never mind, I see that since the default minpoll is 6, this returns an error.

@zhangyanzhao zhangyanzhao added the Triaged this issue has been triaged label Feb 8, 2024
@zhangyanzhao
Copy link
Collaborator

@bsun-sudo can you please help to fix the build failure? Thanks.

@zhangyanzhao zhangyanzhao moved this from To be reviewed to Review in progress in yang Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DELL Triaged this issue has been triaged YANG YANG model related changes
Projects
yang
Review in progress
Development

Successfully merging this pull request may close these issues.

None yet

6 participants