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

Replace cmp in acl_loader with operator.eq #2328

Merged
merged 3 commits into from
Aug 30, 2022

Conversation

ZhaohuiS
Copy link
Contributor

@ZhaohuiS ZhaohuiS commented Aug 23, 2022

Signed-off-by: Zhaohui Sun zhaohuisun@microsoft.com

What I did

When run try to add a new rule for existing rule table with this command acl-loader update incremental, it will throw a traceback like this:

admin@vlab-03:~$ sudo acl-loader update incremental /home/admin/external_acl.json 
Traceback (most recent call last):
  File "/usr/local/bin/acl-loader", line 8, in <module>
    sys.exit(cli())
  File "/usr/local/lib/python3.9/dist-packages/click/core.py", line 764, in __call__
    return self.main(*args, **kwargs)
  File "/usr/local/lib/python3.9/dist-packages/click/core.py", line 717, in main
    rv = self.invoke(ctx)
  File "/usr/local/lib/python3.9/dist-packages/click/core.py", line 1137, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/usr/local/lib/python3.9/dist-packages/click/core.py", line 1137, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/usr/local/lib/python3.9/dist-packages/click/core.py", line 956, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/usr/local/lib/python3.9/dist-packages/click/core.py", line 555, in invoke
    return callback(*args, **kwargs)
  File "/usr/local/lib/python3.9/dist-packages/click/decorators.py", line 17, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "/usr/local/lib/python3.9/dist-packages/acl_loader/main.py", line 1066, in incremental
    acl_loader.incremental_update()
  File "/usr/local/lib/python3.9/dist-packages/acl_loader/main.py", line 761, in incremental_update
    if cmp(self.rules_info[key], self.rules_db_info[key]) != 0:
NameError: name 'cmp' is not defined

cmp is deprecated in python3.

How I did it

  1. Use operator.eq instead.
    The return type of operator.eq is bool, it returns True if x is equal to y, False, otherwise.
    cmp syntax:
    cmp(a, b)
    Parameters:
    a and b are the two numbers in which the comparison is being done.
    Returns:
    -1 if a<b
    0 if a=b
    1 if a>b

  2. Add unit test

How to verify it

Load acl json file with existing rule table in the config file with this command:
acl-loader update incremental**.json

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)

Signed-off-by: Zhaohui Sun <zhaohuisun@microsoft.com>
bingwang-ms
bingwang-ms previously approved these changes Aug 24, 2022
@bingwang-ms
Copy link
Contributor

Please update test case to cover the diff.

prsunny
prsunny previously approved these changes Aug 24, 2022
@@ -758,7 +759,7 @@ def incremental_update(self):
namespace_configdb.mod_entry(self.ACL_RULE, key, None)

for key in existing_controlplane_rules:
if cmp(self.rules_info[key], self.rules_db_info[key]) != 0:
if operator.eq(self.rules_info[key], self.rules_db_info[key]) != 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

operator.eq will return a bool?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@qiluo-msft Yes. The return type of this method is bool, it returns True if x is equal to y, False, otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@qiluo-msft Thank you for your question, for python2, cmp returns 0 if x is equal to y, if not equal, the result is not 0, it should be -1 or 1. So, operator.eq's return result is opposite with cmp's results. I will update code to make the logic right and write unit test case as well.

Syntax:
cmp(a, b)
Parameters:
a and b are the two numbers in which the comparison is being done.
Returns:
-1 if a<b
0 if a=b
1 if a>b

Copy link
Contributor

Choose a reason for hiding this comment

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

I see your new iteration. To make it better, use if not eq().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@qiluo-msft Ye, agree, I updated it.

Signed-off-by: Zhaohui Sun <zhaohuisun@microsoft.com>
@ZhaohuiS ZhaohuiS dismissed stale reviews from prsunny and bingwang-ms via 91d4932 August 29, 2022 04:10
bingwang-ms
bingwang-ms previously approved these changes Aug 29, 2022
Signed-off-by: Zhaohui Sun <zhaohuisun@microsoft.com>
@prsunny
Copy link
Contributor

prsunny commented Sep 1, 2022

@ZhaohuiS , we need this for 202012 as well, correct? Can you please raise a separate PR?

qiluo-msft pushed a commit that referenced this pull request Sep 1, 2022
cmp is deprecated in python3. Use operator.eq instead

Signed-off-by: Zhaohui Sun <zhaohuisun@microsoft.com>
yxieca pushed a commit that referenced this pull request Sep 8, 2022
cmp is deprecated in python3. Use operator.eq instead

Signed-off-by: Zhaohui Sun <zhaohuisun@microsoft.com>
preetham-singh pushed a commit to preetham-singh/sonic-utilities that referenced this pull request Nov 21, 2022
cmp is deprecated in python3. Use operator.eq instead

Signed-off-by: Zhaohui Sun <zhaohuisun@microsoft.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