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

Jump to bottom

Port ipapython.dnssec.odsmgr to xml.etree #241

Closed
wants to merge 2 commits into from
Closed

Port ipapython.dnssec.odsmgr to xml.etree #241

wants to merge 2 commits into from

Conversation

tiran
Copy link
Member

@tiran tiran commented Nov 15, 2016 •
edited by MartinBasti

The module ipapython.dnssec.odsmgr is the only module in ipalib,
ipaclient, ipapython and ipaplatform that uses lxml.etree.

The only dependency on lxml left in [free]ipa-client package is in the
ipa-client-automount script.

https://fedorahosted.org/freeipa/ticket/6469
https://fedorahosted.org/freeipa/ticket/6491

Signed-off-by: Christian Heimes cheimes@redhat.com

MartinBasti
MartinBasti suggested changes Nov 15, 2016
View reviewed changes
ipatests/.cache/v/cache/lastfailed
@@ -0,0 +1 @@
{}
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this?

Copy link
Member Author

Choose a reason for hiding this comment

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

bah, .gitignore is missing a line to ignore .cache directories.

ipatests/test_ipapython/test_dnssec.py
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program. If not, see <http://www.gnu.org/licenses/>.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the new license header

Copy link
Member Author

Choose a reason for hiding this comment

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

Which is the new license header?

MartinBasti
MartinBasti suggested changes Nov 15, 2016
View reviewed changes
freeipa.spec.in
@@ -248,6 +248,7 @@ Requires: %{name}-server-common = %{version}-%{release}
Requires: %{name}-common = %{version}-%{release}
Requires: python2-ipaclient = %{version}-%{release}
Requires: python-ldap >= 2.4.15
Requires: python-lxml
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you move dependency from python-ipalib to python-ipaserver package? You are editing ipapython module which is in python-ipalib package, so dependency should stay in python-ipalib package

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly, I removed the dependency on lxml from ipalib. Only ipaserver depends on lxml.

Copy link
Contributor

Choose a reason for hiding this comment

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

file odsmgr.py is in python2-ipalib, so dependency python-lxml should stay with python2-ipalib not ipaserver

$ rpm -ql python2-ipalib | grep odsmgr
/usr/lib/python2.7/site-packages/ipapython/dnssec/odsmgr.py
/usr/lib/python2.7/site-packages/ipapython/dnssec/odsmgr.pyc
/usr/lib/python2.7/site-packages/ipapython/dnssec/odsmgr.pyo

$ # rpm -ql python2-ipaserver | grep odsmgr
$

You can move the whole dnssec dir/package into ipaserver with all related dependencies (in new patch) or keep python-lxml dependency with python-ipalib

Copy link
Member Author

Choose a reason for hiding this comment

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

You are misunderstanding the patch. odsmgr does no longer use python-lxml. That's the whole point of the patch. xml.etree is part of Python's standard library.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK

@MartinBasti
Copy link
Contributor

I wrote some inline comments, I will test it later, DNSSEC is very hard to debug when an issue occurs, so review must be perfect.

@tiran
Copy link
Member Author

tiran commented Nov 15, 2016

I just replaced the XML parser with a slightly different code. I even included unit test to ensure that the XML parser works. Feel free to add more XML files or a real world example. I couldn't find one.

@tiran
Port ipapython.dnssec.odsmgr to xml.etree
d22d101 
The module ipapython.dnssec.odsmgr is the only module in ipalib,
ipaclient, ipapython and ipaplatform that uses lxml.etree.

https://fedorahosted.org/freeipa/ticket/6469

Signed-off-by: Christian Heimes <cheimes@redhat.com>
@tiran
Use xml.etree in ipa-client-automount script
e6e7e66 
The ipa-client-automount script used lxml.etree to modify
/etc/autofs_ldap_auth.conf.

Signed-off-by: Christian Heimes <cheimes@redhat.com>
@tiran
Copy link
Member Author

tiran commented Nov 16, 2016

client/ipa-client-automount also used lxml.etree. I replaced the implementation with simpler xml.etree based code.

$ find . \( -type f -and -executable \) -or -name '*.py' | xargs grep lxml
./ipaserver/plugins/dogtag.py:To parse the XML documents we use the Python lxml package which is a Python
./ipaserver/plugins/dogtag.py:for many projects. One of the features in lxml and libxml2 that is particularly
./ipaserver/plugins/dogtag.py:namespaces. The regular expression name space identifier is 're:' In lxml we
./ipaserver/plugins/dogtag.py:from lxml import etree
./ipaserver/install/ipa_otptoken_import.py:from lxml import etree
./config.status:S["XMLRPC_LIBS"]="-lxmlrpc_client"

@MartinBasti MartinBasti self-assigned this Nov 16, 2016
@MartinBasti
Copy link
Contributor

Commit has no ticket Use xml.etree in ipa-client-automount script

@MartinBasti MartinBasti added the ack Pull Request approved, can be merged label Nov 16, 2016
@MartinBasti
Copy link
Contributor

There is already DNSSEC issue that is not caused by this PR https://fedorahosted.org/freeipa/ticket/6495

@MartinBasti MartinBasti added the pushed Pull Request has already been pushed label Nov 16, 2016
@MartinBasti
Copy link
Contributor

Fixed upstream
master:
https://fedorahosted.org/freeipa/changeset/64af88fee4a482b3f393d38ff2c7f9494e689a7b
https://fedorahosted.org/freeipa/changeset/9fbd29cc106660865bc6cda225d6a8a338a78d31

@tiran tiran deleted the issue6469_odsmgr_xml_etree branch November 17, 2016 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MartinBasti MartinBasti MartinBasti requested changes

Assignees

@MartinBasti MartinBasti

Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
Milestone
No milestone
2 participants
@tiran @MartinBasti