[sonic-config-engine][portconfig] Do not parse JSON as Python AST #10224
Conversation
vboykox
changed the title
vboykox
force-pushed
the
vboykox/remove-ast-parsing-in-portconfig
branch
from
e0ec6de
to
e542044
Compare
vboykox
changed the title
|
@qiluo-msft @lguohan please review & merge |
1 similar comment
|
@qiluo-msft @lguohan please review & merge |
| @@ -45,16 +46,23 @@ | |||
| # | |||
| # Helper Functions | |||
| # | |||
|
|
|||
| # For python2 compatibility | |||
| def jsonStrHook(j): | |||
There was a problem hiding this comment.
@qiluo-msft
The only reason I added py2 compatibility is failing unit tests for sonic-config-engine py2 package. These are built because ENABLE_PY2_MODULES is yes for buster and jessie. Any reason to keep it as of now?
Regarding non-ascii chars Idk. It's so happens we have an implicit requirement to not have bools which we need for sure. Plus I have not found any platform.json in master having non-ascii. So currently I cannot think of a non-compromise solution. Ideally we just drop py2.
There was a problem hiding this comment.
If we really need this hook for python2, can we add it only for python2? Python3 just has no hook at all.
There was a problem hiding this comment.
Alternatively in json we get from json.load we can replace bool values to some string makers and then replace the markers back with False or True before passing to ast.literal_eval. This also would keep compatibility with non-ascii.
@qiluo-msft But do we really need py2 compatibility? What are the plans for buster?
There was a problem hiding this comment.
@saiarcot895 Could you help answer the py2 plan on buster?
There was a problem hiding this comment.
Buster will continue to have Python 2 available, but it won't necessarily be installed by default (only if something needs it).
However, from what I can tell, rules/docker-config-engine-buster.mk is using the python 3 version of sonic-config-engine, so this might not be needed on Buster containers. (I verified on a couple Buster containers that sonic-cfggen is calling the Python 3 interpreter.)
Stretch is using the python 2 version of sonic-config-engine. Looks like the syncd container for innovium and nephos are on Stretch.
| @@ -45,16 +46,23 @@ | |||
| # | |||
| # Helper Functions | |||
| # | |||
|
|
|||
| # For python2 compatibility | |||
There was a problem hiding this comment.
I made changes addressing your comments
- on non-ascii chars - I kept the behavior the same as previous - instead of throwing UnicodeEcodeError, encode with hex escapes.
- moved the compatibility code under sys.version_info.major == 2 check
- added bool values to sample_platform.json. The changes are already covered by test_cfggen_platformJson
There was a problem hiding this comment.
@qiluo-msft please review
There was a problem hiding this comment.
@qiluo-msft please review
There was a problem hiding this comment.
@qiluo-msft @saiarcot895 thanks
vboykox
force-pushed
the
vboykox/remove-ast-parsing-in-portconfig
branch
4 times, most recently
from
b0cb9ae
to
35e62b5
Compare
Signed-off-by: Volodymyr Boyko <volodymyrx.boiko@intel.com>
qiluo-msft
added
Request for 202106 Branch
Request for 202111 Branch
…0224) #### Why I did it To fix #9643 #### How I did it Instead of ast.literal_eval added python2 compat code for json strings unicode -> str convertion. We need python2 compatibility since py2 sonic config engine (buster/sonic_config_engine-1.0-py2-none-any.whl target) is still included into the build (ENABLE_PY2_MODULES flag is set for buster). Once we abandon buster and python2, this compat and ast.literal_eval could be cleaned up all through the code base. #### How to verify it run steps from the linked issue
…0224) #### Why I did it To fix #9643 #### How I did it Instead of ast.literal_eval added python2 compat code for json strings unicode -> str convertion. We need python2 compatibility since py2 sonic config engine (buster/sonic_config_engine-1.0-py2-none-any.whl target) is still included into the build (ENABLE_PY2_MODULES flag is set for buster). Once we abandon buster and python2, this compat and ast.literal_eval could be cleaned up all through the code base. #### How to verify it run steps from the linked issue
…202012 Due to confliction in files/image_config/platform/rc.local Related work items: sonic-net#9248, sonic-net#10224, sonic-net#10923, sonic-net#12164, sonic-net#12775, sonic-net#12796, sonic-net#12806, sonic-net#12808
…nic-net#10224) Signed-off-by: Sangita Maity <samaity@linkedin.com> To fix sonic-net#9643 Instead of ast.literal_eval added python2 compat code for json strings unicode -> str convertion. We need python2 compatibility since py2 sonic config engine (buster/sonic_config_engine-1.0-py2-none-any.whl target) is still included into the build (ENABLE_PY2_MODULES flag is set for buster). Once we abandon buster and python2, this compat and ast.literal_eval could be cleaned up all through the code base. run steps from the linked issue
Signed-off-by: Volodymyr Boyko volodymyrx.boiko@intel.com
Why I did it
To fix #9643
How I did it
Instead of ast.literal_eval added python2 compat code for json strings unicode -> str convertion.
We need python2 compatibility since py2 sonic config engine (buster/sonic_config_engine-1.0-py2-none-any.whl target) is still included into the build (ENABLE_PY2_MODULES flag is set for buster). Once we abandon buster and python2, this compat and ast.literal_eval could be cleaned up all through the code base.
How to verify it
run steps from the linked issue
Which release branch to backport (provide reason below if selected)
Description for the changelog
Link to config_db schema for YANG module changes
A picture of a cute animal (not mandatory but encouraged)