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

[Bug-fix] ovs-monitor-ipsec Python2 code to Python3 #331

Closed
wants to merge 1 commit into from

Conversation

lzhecheng
Copy link
Contributor

@lzhecheng lzhecheng commented Aug 6, 2020

I am not a skilled Python programmer. I made these changes with 2to3 and debugging in another downstream project. Thanks.
Fixes: openvswitch/ovs-issues#192

Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

you don't need to build lists for your iterations. You can iterate over the result of items(), keys(), values() directly.

The reason why Python 2to3 is translating this:

        for name, tunnel in self.tunnels.iteritems():

to that:

        for name, tunnel in list(self.tunnels.items()):

Even though it is clear from the Python2 code that you are doing an iteration, is that the semantics of items in Pyhton3 are different from the ones of iteritems in Python2: faucetsdn/faucet#2372 (comment). But it only matters if you are modifying the dictionary while iterating over it.

So in a nutshell I think you can / should remove all the calls to list() from your patch.

@lzhecheng
Copy link
Contributor Author

you don't need to build lists for your iterations. You can iterate over the result of items(), keys(), values() directly.

The reason why Python 2to3 is translating this:

        for name, tunnel in self.tunnels.iteritems():

to that:

        for name, tunnel in list(self.tunnels.items()):

Even though it is clear from the Python2 code that you are doing an iteration, is that the semantics of items in Pyhton3 are different from the ones of iteritems in Python2: faucetsdn/faucet#2372 (comment). But it only matters if you are modifying the dictionary while iterating over it.

So in a nutshell I think you can / should remove all the calls to list() from your patch.

You are right, thank you! Code updated.

@igsilya
Copy link
Member

igsilya commented Aug 13, 2020

Thanks for the patch.

We need a Signed-off-by for this patch. The FAQ says:

Q: What's a Signed-off-by and how do I provide one?

A: Free and open source software projects usually require a contributor to
provide some assurance that they're entitled to contribute the code that
they provide.  Some projects, for example, do this with a Contributor
License Agreement (CLA) or a copyright assignment that is signed on paper
or electronically.

For this purpose, Open vSwitch has adopted something called the Developer's
Certificate of Origin (DCO), which is also used by the Linux kernel and
originated there.  Informally stated, agreeing to the DCO is the
developer's way of attesting that a particular commit that they are
contributing is one that they are allowed to contribute.  You should visit
https://developercertificate.org/ to read the full statement of the DCO,
which is less than 200 words long.

To certify compliance with the Developer's Certificate of Origin for a
particular commit, just add the following line to the end of your commit
message, properly substituting your name and email address:

    Signed-off-by: Firstname Lastname <email@example.org>

Git has special support for adding a Signed-off-by line to a commit
message: when you run "git commit", just add the -s option, as in "git
commit -s".  If you use the "git citool" GUI for commits, you can add a
Signed-off-by line to the commit message by pressing Control+S.  Other Git
user interfaces may provide similar support.

Signed-off-by: lzhecheng <lzhecheng@vmware.com>
@lzhecheng
Copy link
Contributor Author

Thanks for the patch.

We need a Signed-off-by for this patch. The FAQ says:

Q: What's a Signed-off-by and how do I provide one?

A: Free and open source software projects usually require a contributor to
provide some assurance that they're entitled to contribute the code that
they provide.  Some projects, for example, do this with a Contributor
License Agreement (CLA) or a copyright assignment that is signed on paper
or electronically.

For this purpose, Open vSwitch has adopted something called the Developer's
Certificate of Origin (DCO), which is also used by the Linux kernel and
originated there.  Informally stated, agreeing to the DCO is the
developer's way of attesting that a particular commit that they are
contributing is one that they are allowed to contribute.  You should visit
https://developercertificate.org/ to read the full statement of the DCO,
which is less than 200 words long.

To certify compliance with the Developer's Certificate of Origin for a
particular commit, just add the following line to the end of your commit
message, properly substituting your name and email address:

    Signed-off-by: Firstname Lastname <email@example.org>

Git has special support for adding a Signed-off-by line to a commit
message: when you run "git commit", just add the -s option, as in "git
commit -s".  If you use the "git citool" GUI for commits, you can add a
Signed-off-by line to the commit message by pressing Control+S.  Other Git
user interfaces may provide similar support.

Thank you. I have updated the commit message.

igsilya pushed a commit to igsilya/ovs that referenced this pull request Aug 17, 2020
Submitted-at: openvswitch#331
Reported-at: openvswitch/ovs-issues#192
Fixes: 1ca0323 ("Require Python 3 and remove support for Python 2.")
Signed-off-by: lzhecheng <lzhecheng@vmware.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
igsilya pushed a commit to igsilya/ovs that referenced this pull request Aug 17, 2020
Submitted-at: openvswitch#331
Reported-at: openvswitch/ovs-issues#192
Fixes: 1ca0323 ("Require Python 3 and remove support for Python 2.")
Signed-off-by: lzhecheng <lzhecheng@vmware.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
igsilya pushed a commit to igsilya/ovs that referenced this pull request Aug 17, 2020
Submitted-at: openvswitch#331
Reported-at: openvswitch/ovs-issues#192
Fixes: 1ca0323 ("Require Python 3 and remove support for Python 2.")
Signed-off-by: lzhecheng <lzhecheng@vmware.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
@igsilya
Copy link
Member

igsilya commented Aug 17, 2020

Thanks! I applied this patch to master and backported down to 2.13.

@igsilya igsilya closed this Aug 17, 2020
markdgray pushed a commit to markdgray/ovs that referenced this pull request Aug 25, 2020
Submitted-at: openvswitch#331
Reported-at: openvswitch/ovs-issues#192
Fixes: 1ca0323 ("Require Python 3 and remove support for Python 2.")
Signed-off-by: lzhecheng <lzhecheng@vmware.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Still some Python2 code in ovs-monitor-ipsec
3 participants