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

[minigraph.py]: Prefer parsing device type from <ElementType> #6184

Merged
merged 2 commits into from
Dec 15, 2020

Conversation

theasianpianist
Copy link
Contributor

@theasianpianist theasianpianist commented Dec 10, 2020

Signed-off-by: Lawrence Lee [email protected]

- Why I did it
In some minigraph deployments, the type attribute in the <Device> tag (<PngDec> -> <Devices>) sometimes includes the XML namespace (e.g. instead of "BackEndLeafRouter" the value is "a:BackEndLeafRouter").

- How I did it
When parsing devices, prefer the <ElementType> tag usually found inside of <Device>, and use the type attribute as a fallback in case it is not found.

- How to verify it
Run the sonic-config-engine tests

- Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)
image

@prsunny
Copy link
Contributor

prsunny commented Dec 10, 2020

@theasianpianist , can you please update the PR heading, it doesn't match the changes?

* Fall back to <Device> type attribute if no <ElementType> is found

Signed-off-by: Lawrence Lee <[email protected]>
@theasianpianist theasianpianist changed the title [minigraph.py]: Remove prefix from peer switch loopback address [minigraph.py]: Prefer parsing device type from <ElementType> Dec 10, 2020
@theasianpianist
Copy link
Contributor Author

retest baseimage please

prsunny
prsunny previously approved these changes Dec 11, 2020
Copy link
Contributor

@prsunny prsunny left a comment

Choose a reason for hiding this comment

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

lgtm, please wait for @lguohan's approval

@theasianpianist
Copy link
Contributor Author

retest vsimage please

@lguohan
Copy link
Collaborator

lguohan commented Dec 11, 2020

I remember the name space is different

@theasianpianist
Copy link
Contributor Author

I remember the name space is different

I'm not sure I understand, do you mean the namespace used in minigraph.py to parse the type? Or the namespace used in the sample minigraph?

@theasianpianist
Copy link
Contributor Author

retest vsimage please

@lguohan
Copy link
Collaborator

lguohan commented Dec 12, 2020

Device xmlns:a="http://schemas.datacontract.org/2004/07/Microsoft.Search.Autopilot.Evolution" i:type="a:BackEndLeafRouter">
<ElementType>BackEndLeafRouter</ElementType>
<Address xmlns:b="Microsoft.Search.Autopilot.NetMux">
</Address>

@lguohan
Copy link
Collaborator

lguohan commented Dec 12, 2020

here is what i see in the example, it seems different from you test example.

@lguohan
Copy link
Collaborator

lguohan commented Dec 14, 2020

it looks like you are using a different approach to address the proble, which is fine. wonder if you can update the sample graph to inlcude the proper namespace so that we can make sure it address the issue.

@theasianpianist
Copy link
Contributor Author

I will update it for consistency. However I don't believe the specific namespace included in the type attribute is important, since the entire attribute is treated as a string by the XML parser.

@theasianpianist theasianpianist marked this pull request as ready for review December 15, 2020 18:19
@theasianpianist theasianpianist merged commit 290f66b into sonic-net:master Dec 15, 2020
@theasianpianist theasianpianist deleted the minigraph-parse-fix branch December 15, 2020 18:20
abdosi pushed a commit that referenced this pull request Dec 19, 2020
* Parse device type from <ElementType> first in <PngDec>
* Fall back to <Device> type attribute if no <ElementType> is found

Signed-off-by: Lawrence Lee <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants