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

[cfg engine] Add support to read device description xml #775

Merged
merged 2 commits into from
Jul 6, 2017

Conversation

taoyl-ms
Copy link
Contributor

No description provided.

name = node.text
elif node.tag == str(QName(ns, "HwSku")):
hwsku = node.text
return (lo_prefix, mgmt_prefix, name, hwsku, d_type)
Copy link
Collaborator

@qiluo-msft qiluo-msft Jul 1, 2017

Choose a reason for hiding this comment

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

name [](start = 36, length = 4)

name may be not itialized. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.


In reply to: 125150495 [](ancestors = 125150495)

@@ -47,6 +47,26 @@ def default(self, obj):
return str(obj)
return json.JSONEncoder.default(self, obj)

def parse_device(device):
lo_prefix = None
Copy link
Collaborator

@qiluo-msft qiluo-msft Jul 1, 2017

Choose a reason for hiding this comment

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

None [](start = 16, length = 4)

suggest init as empty string, so later conditional split can be simplified. #ByDesign

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's keep it None as returning an empty string might make template debug mroe difficult than None.


In reply to: 125150571 [](ancestors = 125150571)

(lo_prefix, mgmt_prefix, hostname, hwsku, d_type) = parse_device(root)

Tree = lambda: defaultdict(Tree)
results = Tree()
Copy link
Collaborator

@qiluo-msft qiluo-msft Jul 1, 2017

Choose a reason for hiding this comment

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

results = {} ? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.


In reply to: 125150653 [](ancestors = 125150653)

results['inventory_hostname'] = hostname

lo_intfs = []
ipn = ipaddress.IPNetwork(lo_prefix)
Copy link
Collaborator

@qiluo-msft qiluo-msft Jul 1, 2017

Choose a reason for hiding this comment

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

lo_prefix [](start = 30, length = 9)

handle exception? #WontFix

Copy link
Contributor Author

@taoyl-ms taoyl-ms Jul 3, 2017

Choose a reason for hiding this comment

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

I'm feeling letting the exception to be thrown out and crashing at the outermost layer is the best behavior if exception happens here.


In reply to: 125150698 [](ancestors = 125150698)

results['minigraph_lo_interfaces'] = lo_intfs

mgmt_intf = None
mgmtipn = ipaddress.IPNetwork(mgmt_prefix)
Copy link
Collaborator

@qiluo-msft qiluo-msft Jul 1, 2017

Choose a reason for hiding this comment

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

mgmtipn [](start = 4, length = 7)

mgmt_ipn? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.


In reply to: 125150747 [](ancestors = 125150747)

@@ -47,7 +48,9 @@ def unique_name(l):

def main():
parser=argparse.ArgumentParser(description="Render configuration file from minigraph data and jinja2 template.")
parser.add_argument("-m", "--minigraph", help="minigraph xml file")
group = parser.add_mutually_exclusive_group()
Copy link
Collaborator

@qiluo-msft qiluo-msft Jul 1, 2017

Choose a reason for hiding this comment

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

() [](start = 47, length = 2)

If one is required, you may use 'required=True' #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's not true that one of them is required.


In reply to: 125150833 [](ancestors = 125150833)

@qiluo-msft
Copy link
Collaborator

qiluo-msft commented Jul 1, 2017

Reformat it so human can read it? #Closed


Refers to: src/sonic-config-engine/tests/device.xml:2 in 7a81ec0. [](commit_id = 7a81ec0, deletion_comment = False)

@taoyl-ms
Copy link
Contributor Author

taoyl-ms commented Jul 3, 2017

Fixed.


In reply to: 312401152 [](ancestors = 312401152)


Refers to: src/sonic-config-engine/tests/device.xml:2 in 7a81ec0. [](commit_id = 7a81ec0, deletion_comment = False)

parser.add_argument("-m", "--minigraph", help="minigraph xml file")
group = parser.add_mutually_exclusive_group()
group.add_argument("-m", "--minigraph", help="minigraph xml file")
group.add_argument("-M", "--device-description", help="device description xml file")
Copy link
Collaborator

Choose a reason for hiding this comment

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

group.add_argument("-M", "--device-description", help="device description xml file") [](start = 1, length = 87)

instead of adding a new option, can we treat the device xml as a variant(subset) of minigraph which only contains a subset of information?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't see your point. Those are different files with different schema. I already make the output format consistence so you can use same tool and same template to generate, for example, interfaces file.

@taoyl-ms taoyl-ms merged commit 90f21d4 into sonic-net:master Jul 6, 2017
lguohan added a commit to lguohan/sonic-buildimage that referenced this pull request Jan 24, 2021
* 20b9573 2021-01-24 | [SAI]: update SAI submodule (sonic-net#775) (HEAD, origin/master, origin/HEAD) [lguohan]
* 667c33d 2021-01-22 | [syncd] Comparison logic add support to LABEL attribute with higher priority (sonic-net#764) [Kamil Cudnik]
* aaf5b98 2021-01-22 | [vslib]: Fix missing MACsec Create Port action (sonic-net#770) [Ze Gan]

Signed-off-by: Guohan Lu <lguohan@gmail.com>
@lguohan lguohan mentioned this pull request Jan 24, 2021
4 tasks
lguohan added a commit that referenced this pull request Jan 25, 2021
* 20b9573 2021-01-24 | [SAI]: update SAI submodule (#775) (HEAD, origin/master, origin/HEAD) [lguohan]
* 667c33d 2021-01-22 | [syncd] Comparison logic add support to LABEL attribute with higher priority (#764) [Kamil Cudnik]
* aaf5b98 2021-01-22 | [vslib]: Fix missing MACsec Create Port action (#770) [Ze Gan]

Signed-off-by: Guohan Lu <lguohan@gmail.com>
lguohan added a commit that referenced this pull request Jan 25, 2021
* 20b9573 2021-01-24 | [SAI]: update SAI submodule (#775) (HEAD, origin/master, origin/HEAD) [lguohan]
* 667c33d 2021-01-22 | [syncd] Comparison logic add support to LABEL attribute with higher priority (#764) [Kamil Cudnik]
* aaf5b98 2021-01-22 | [vslib]: Fix missing MACsec Create Port action (#770) [Ze Gan]

Signed-off-by: Guohan Lu <lguohan@gmail.com>
AidanCopeland pushed a commit to Metaswitch/sonic-buildimage that referenced this pull request Apr 14, 2022
* c0bdac2 2021-01-24 | [saithrift]: add correct libs to build saithrift on libsaivs (sonic-net#1184) (HEAD, origin/v1.7) [lguohan]

Signed-off-by: Guohan Lu <lguohan@gmail.com>
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.

5 participants