-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
chore: remove warnings from defusedxml package #32652
chore: remove warnings from defusedxml package #32652
Conversation
Thanks for the pull request, @diegovr! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
2e9b66b
to
9e93737
Compare
@e0d could you please enable the tests to run 🙂 |
1db8c97
to
adb8f49
Compare
@diegovr looks like there are some test and linting issues, can you have a look? |
7838ebc
to
2f1b31c
Compare
Hi @e0d I have successfully reviewed the accurate unit tests. However, the most time-consuming aspect was dealing with the linting issues. Strangely, they seem to take a considerable amount of time to execute when running locally. By the way, I wanted to inquire if there is an ongoing issue with the tests again, do I need to inform you each time in order to initiate the testing process? I don't want to bother you. |
e99fd85
to
f9909c0
Compare
f9909c0
to
d7709f5
Compare
@e0d In the last checks I found just one lintin issue. I solved it. Could you please enable again the tests to run? |
@e0d - All checks passed as expected, the pipeline is 🟢 :) |
@ormsbee any thoughts on best reviewers? |
@dianakhuang: Is this in your wheelhouse? (I have this vague recollection that you worked on this, but I might be totally off base.) |
I don't think I worked on this originally, but I do think that I am, unfortunately, the last person to have touched this code on the 2U side. |
@dianakhuang just a friendly reminder for reviewing this PR please 👍 |
Thanks for the ping! I think it looks reasonable, and I'm happy to give it an approval. I'm curious about the follow up work to remove |
@dianakhuang , thanks for taking some time to review the PR. As far as I know the issue is related to remove the DeprecationWarnings that belong to the |
Thank you @dianakhuang to approve it. So, I think the PR is ready to be merged. |
@diegovr 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
2U Release Notice: This PR has been deployed to the edX production environment. |
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
2U Release Notice: This PR has been deployed to the edX production environment. |
Description
Issue: openedx/public-engineering#146
defusedxml
package which will no longer be supported.python3.8
) supports thexml.etree.ElementTree
package which replace the old package (defusedxml
).defusedxml
package.Supporting information
The best references to why it was deprecated I could find are here:
Also according to this documentation, since Python 3.7.1, xml modules: are no longer processes general external entities by default to increase security by default.
Testing instructions
To check and find from where I get the same warning, I run the unit tests inside the
lms
container (by runningmake dev.shell.lms
) and I could found that the tests which use the deprecated package are fromopenedx/core/lib/tests/
path so I usedpytest
to confirm that.Then after the changes I re-run the tests and the warning no longer appeared.
Other information
defusedxml
package since is used only in this section.