X Tutup
The Wayback Machine - https://web.archive.org/web/20201015173033/https://github.com/certbot/certbot/pull/8020
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

OS umask Context Manager #8020

Open
wants to merge 9 commits into
base: master
from
Open

OS umask Context Manager #8020

wants to merge 9 commits into from

Conversation

@Mercerenies
Copy link

@Mercerenies Mercerenies commented May 26, 2020

This PR addresses #7992, creating a context manager in certbot.util and implementing its use in the places the os.umask / finally pattern is used in the current codebase.

Pull Request Checklist

  • If the change being made is to a distributed component, edit the master section of certbot/CHANGELOG.md to include a description of the change being made.
  • Include your name in AUTHORS.md if you like.
Copy link
Contributor

@m0namon m0namon left a comment

Thank you for your PR! This pattern occurs in certbot_apache/http_01.py as well as certbot/compat/filesystem.py if you'd like to use this there as well!

@Mercerenies
Copy link
Author

@Mercerenies Mercerenies commented May 27, 2020

Python 2 seems to be acting up when I try to import certbot.util in those files. Python 3 is fine with it. In the filesystem.py case, it seems to be some kind of circular import issue (certbot.util imports certbot._internal.lock, which imports certbot.compat.filesystem, which tries to import certbot.util). Not sure what http_01.py is doing. I'll take another look at it.

====================================================== ERRORS ======================================================
__________________________________ ERROR collecting certbot/tests/account_test.py __________________________________
ImportError while importing test module '/home/silvio/Documents/python/certbot/certbot/tests/account_test.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
certbot/tests/account_test.py:15: in <module>
    from certbot.compat import filesystem
certbot/certbot/compat/filesystem.py:9: in <module>
    from certbot import util
certbot/certbot/util.py:25: in <module>
    from certbot._internal import lock
certbot/certbot/_internal/lock.py:7: in <module>
    from certbot.compat import filesystem
E   ImportError: cannot import name filesystem
@bmw
Copy link
Member

@bmw bmw commented Jun 9, 2020

To fix the Travis failures, you need to update this line to certbot>=1.6.0.dev0 and this line to -e certbot[dev].

Whats going on here is the tests are trying to run certbot-apache with the oldest version of certbot it claims to support which is currently version 1.1.0, but this version doesn't have certbot.util.os_umask being added in this PR. Updating the minimum required version should solve this problem.

Documenting how all this works is tracked by #7647.

@kden
Copy link
Contributor

@kden kden commented Jun 9, 2020

@bmw are you saying we add in that change to that line to the pull request even though it might affect things beyond #8020?

@bmw
Copy link
Member

@bmw bmw commented Jun 9, 2020

I'm not entirely sure what you have in mind when you say it might affect things beyond this PR, but yes, I think the changes I described should be made in this PR. This PR is making our Apache plugin require the latest version of Certbot which I think is the best approach, but tests will fail until the Apache plugin's dependencies are updated to state this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.
X Tutup