[config][generic-update] Implementing patch sorting#1599

Merged

ghooo merged 4 commits intosonic-net:masterfrom

ghooo:dev/mghoneim/patch_sorter

Aug 10, 2021

Contributor

ghooo commented May 6, 2021

What I did

Implemented JSON Patch Ordering using YANG Models Design Doc

How I did it

How to verify it

Unit-Tests

Previous command output (if the output of a command-line utility has changed)

New command output (if the output of a command-line utility has changed)

lgtm-com bot commented May 6, 2021

This pull request introduces 10 alerts when merging ba2b1d6bf6a1ddf011327887a697d89b778e1dea into 9a88cb6 - view on LGTM.com

new alerts:

4 for Unused import
3 for Unused local variable
2 for Testing equality to None
1 for Inconsistent equality and hashing

lgtm-com bot commented May 17, 2021

This pull request introduces 10 alerts when merging 97bc24c4c75dc1b37ecf7bd9d79337c71aba7c08 into ad801bf - view on LGTM.com

new alerts:

5 for Unused import
2 for Testing equality to None
2 for Unused local variable
1 for Inconsistent equality and hashing

Contributor

qiluo-msft commented May 17, 2021 •

edited

Loading

Please fix LGTM alerts first.

In reply to: 842690595

In reply to: 842690595

lgtm-com bot commented May 18, 2021

This pull request introduces 1 alert when merging d61839670813af4455476e788c10fa70280cca82 into ad801bf - view on LGTM.com

new alerts:

1 for Unused import

ghooo force-pushed the dev/mghoneim/patch_sorter branch 3 times, most recently from 2cad759 to c20679f Compare

May 20, 2021 11:10

qiluo-msft reviewed

View reviewed changes

generic_config_updater/patch_sorter.py Outdated

Contributor

qiluo-msft May 21, 2021 •

edited

Loading

Remove unused code or comments. #Closed

Contributor Author

ghooo May 24, 2021

updated

qiluo-msft reviewed

View reviewed changes

generic_config_updater/patch_sorter.py Outdated

Contributor

qiluo-msft May 21, 2021 •

edited

Loading

hash(cc) ^ hash(tc)

How about hash ((cc, tc))?
The benefit is swapping input will result in different value. #Closed

Contributor Author

ghooo May 24, 2021

good idea. updated

qiluo-msft reviewed

View reviewed changes

tests/generic_config_updater/gu_common_test.py Outdated

Contributor

qiluo-msft May 21, 2021 •

edited

Loading

remove unused code. #Closed

Contributor Author

ghooo May 24, 2021

updated

qiluo-msft reviewed

View reviewed changes

generic_config_updater/gu_common.py Outdated

Contributor

qiluo-msft May 26, 2021 •

edited

Loading

JSON Pointer

# meanings in JSON Pointer, '~' needs to be encoded as '~0' and '/'

Did you try https://pypi.org/project/jsonpointer/ ? #Closed

Contributor Author

ghooo Jun 22, 2021

still looking into this

Contributor Author

ghooo Aug 9, 2021

updated

qiluo-msft reviewed

View reviewed changes

generic_config_updater/gu_common.py Outdated

Contributor

qiluo-msft May 26, 2021 •

edited

Loading

or ly.LYS_LEAFLIST == schema.nodetype()

return ly.LYS_LEAF == schema.nodetype() # or ly.LYS_LEAFLIST == schema.nodetype()

Remove unused code? #Closed

Contributor Author

ghooo Jun 22, 2021

updated

qiluo-msft reviewed

View reviewed changes

generic_config_updater/gu_common.py Outdated

Contributor

qiluo-msft May 26, 2021 •

edited

Loading

==

if xpath=="/": # Point to Root element which contains all xpaths

Add blanks before and after == #Closed

Contributor Author

ghooo Jun 22, 2021

updated

qiluo-msft reviewed

View reviewed changes

generic_config_updater/gu_common.py Outdated

Contributor

qiluo-msft May 26, 2021 •

edited

Loading

# It is OK to have null backlinks if there is no backlinks.

Could you create a Github issue or submit a PR if you already know how to fix? #Closed

Contributor Author

ghooo Jun 22, 2021 •

edited

Loading

~~PR submitted: sonic-net/sonic-buildimage#7939~~

Contributor Author

ghooo Jun 22, 2021

NOTE: unit-tests in the current PR (patch-sorting) will fail until the PR submitted above is completed

Contributor Author

ghooo Jul 15, 2021

created PR on forked repo: sonic-net/sonic-buildimage#8187

qiluo-msft reviewed

View reviewed changes

generic_config_updater/gu_common.py Outdated

Contributor

qiluo-msft May 26, 2021 •

edited

Loading

Exception

except Exception as e:

Could you use specific exception type? Here you swallow all the exception types and lose the exception context (like stack). #Closed

Contributor Author

ghooo Jun 22, 2021 •

edited

Loading

code removed locally

Contributor Author

ghooo Jun 22, 2021

code removed

qiluo-msft reviewed

View reviewed changes

generic_config_updater/gu_common.py Outdated

Contributor

qiluo-msft May 26, 2021 •

edited

Loading

Exception

except Exception as e:

Could you use specific exception type? Seems you know the exception is related to "Failed to find node or dependencies" #Closed

Contributor Author

ghooo Jun 22, 2021 •

edited

Loading

code removed locally

Contributor Author

ghooo Jun 22, 2021

code removed

qiluo-msft reviewed

View reviewed changes

generic_config_updater/gu_common.py Outdated

Contributor

qiluo-msft Jun 6, 2021 •

edited

Loading

str

mod_token = str(token)

Why you need to str convert? #Closed

Contributor Author

ghooo Jun 22, 2021

good catch, updated locally

Contributor Author

ghooo Jun 22, 2021

updated

qiluo-msft reviewed

View reviewed changes

generic_config_updater/gu_common.py Outdated

Contributor

qiluo-msft Jun 6, 2021 •

edited

Loading

{token}[.='{value}

It's difficult to read, could you add some comment? Is the . a mistake? #Closed

Contributor Author

ghooo Jun 22, 2021

To get a leaf-list instance with the value 'val'
/module-name:container/leaf-list[.='val']

Source: Check examples in https://netopeer.liberouter.org/doc/libyang/master/html/howto_x_path.html

Added a comment to explain this '.' locally.

Contributor Author

ghooo Jun 22, 2021

updated

qiluo-msft reviewed

View reviewed changes

generic_config_updater/gu_common.py Outdated

Contributor

qiluo-msft Jun 6, 2021 •

edited

Loading

and clist['@name'] == token+"_LIST":

if isinstance(clist, dict): # and clist['@name'] == token+"_LIST":

Remove unused code? #Closed

Contributor Author

ghooo Jun 22, 2021

removed

qiluo-msft reviewed

View reviewed changes

generic_config_updater/gu_common.py Outdated

Contributor

qiluo-msft Jun 6, 2021 •

edited

Loading

nxt_token

nxt_token = xpath_tokens[token_index+1]

Do you mean next_token? #Closed

Contributor Author

ghooo Jun 22, 2021

updated

qiluo-msft reviewed

View reviewed changes

generic_config_updater/patch_sorter.py Outdated

Contributor

qiluo-msft Jun 6, 2021 •

edited

Loading

=

self.patch=jsonpatch.JsonPatch([operation])

Add a blank before and after operators. #Closed

Contributor Author

ghooo Jun 22, 2021

added

qiluo-msft reviewed

View reviewed changes

generic_config_updater/patch_sorter.py Outdated

Contributor

qiluo-msft Jun 6, 2021 •

edited

Loading

hash(self.op_type) ^ hash(self.path) ^ hash(json.dumps(self.value))

return hash(self.op_type) ^ hash(self.path) ^ hash(json.dumps(self.value))

hash((a, b, c)) ? #Closed

Contributor Author

ghooo Jun 22, 2021

updated

qiluo-msft reviewed

View reviewed changes

Contributor

qiluo-msft left a comment

As comments

ghooo added 3 commits

July 17, 2021 03:38


          [config][generic-update] Implementing patch sorting

4da6ffa


          addressing comments


          addressing comments

c554f6d

ghooo force-pushed the dev/mghoneim/patch_sorter branch from d3d04d2 to c554f6d Compare

July 17, 2021 10:38


          using jsonpointer to get path parts

9590bdc

qiluo-msft approved these changes

View reviewed changes

ghooo merged commit 920bb87 into sonic-net:master

vivekrnv added a commit to vivekrnv/sonic-utilities that referenced this pull request


          Revert "[config][generic-update] Implementing patch sorting (sonic-ne…

e6122e9

…t#1599)"

This reverts commit 920bb87.

lguohan added a commit that referenced this pull request


          Revert "[config][generic-update] Implementing patch sorting (#1599)"

20abc03

This reverts commit 920bb87.

lguohan added a commit that referenced this pull request


          Revert "[config][generic-update] Implementing patch sorting (#1599)"

0a145e8

This reverts commit 920bb87.

Contributor

lguohan commented Aug 17, 2021 •

edited

Loading

@ghooo , please note since this pr is causing unit testing failure, it has been reverted. Please check the error here. https://dev.azure.com/mssonic/build/_build/results?buildId=29578&view=logs&j=cef3d8a9-152e-5193-620b-567dc18af272&t=359769c4-8b5e-5976-a793-85da132e0a6f&s=ff05ad62-bb9a-53b6-ce9f-72f329a63e7c

please check attempt #1

vivekrnv added a commit to vivekrnv/sonic-utilities that referenced this pull request


          Revert "Revert "[config][generic-update] Implementing patch sorting (s…

903a2f0

…onic-net#1599)""

This reverts commit e6122e9.

This was referenced Sep 1, 2021

[config][generic-update] Implementing patch sorting (2nd attempt) #1794

Merged

[sonic-utilities] Unit test failed when building sonic-utilities #1761

Closed

qiluo-msft pushed a commit that referenced this pull request


          [config][generic-update] Implementing patch sorting (2nd attempt) (#1794

d7900dd

)

#### What I did
Implemented [JSON Patch Ordering using YANG Models Design Doc](https://github.com/Azure/SONiC/blob/master/doc/config-generic-update-rollback/Json_Patch_Ordering_using_YANG_Models_Design.md)

#### How to verify it
Unit-Tests

**NOTE: The code in this PR was [reverted](github.com/Azure/sonic-utilities/commit/0a145e8027380e8d4decb36bdfc647062c722612) before because of some [build issues](#1761). Build issues have been fixed [here](sonic-net/sonic-buildimage#8632). To check the original PR comments please go [here](#1599

ghooo mentioned this pull request

SONiC Generic Update and Rollback - HLD sonic-net/SONiC#736

Merged

malletvapid23 added a commit to malletvapid23/Sonic-Utility that referenced this pull request


          [config][generic-update] Implementing patch sorting (2nd attempt) (#1…

5c02fd3

…794)

#### What I did
Implemented [JSON Patch Ordering using YANG Models Design Doc](https://github.com/Azure/SONiC/blob/master/doc/config-generic-update-rollback/Json_Patch_Ordering_using_YANG_Models_Design.md)

#### How to verify it
Unit-Tests

**NOTE: The code in this PR was [reverted](github.com/Azure/sonic-utilities/commit/0a145e8027380e8d4decb36bdfc647062c722612) before because of some [build issues](sonic-net/sonic-utilities#1761). Build issues have been fixed [here](sonic-net/sonic-buildimage#8632). To check the original PR comments please go [here](sonic-net/sonic-utilities#1599

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet