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

Fix umask option bug(issues#1622) #1632

Merged
merged 2 commits into from Oct 28, 2017

Conversation

Projects
None yet
2 participants
@hramezani
Contributor

hramezani commented Oct 24, 2017

Fix umask option bug. issue_1622

@berkerpeksag

Could you also convert the example shared in #1622 to a test case?

if x.startswith('0') and not x.lower().startswith('0x'):
# for compatible with octal numbers in python3
# for compatible with octal numbers in python3
if re.match('0(\d)', x, re.IGNORECASE):

This comment has been minimized.

@berkerpeksag

berkerpeksag Oct 26, 2017

Collaborator

I'd use r'' to avoid subtle bugs.

@berkerpeksag berkerpeksag referenced this pull request Oct 26, 2017

Closed

release 19.8.0 #1634

@hramezani

This comment has been minimized.

Contributor

hramezani commented Oct 27, 2017

@berkerpeksag thanks, I add those examples to test_umask_config.

@berkerpeksag

This comment has been minimized.

Collaborator

berkerpeksag commented Oct 27, 2017

I think the problem here is that our test code doesn't call auto_int so even if we already added a test for --umask 0 we missed this bug.

@berkerpeksag

This comment has been minimized.

Collaborator

berkerpeksag commented Oct 27, 2017

Nevermind, the existing test was wrong.

@@ -375,6 +375,8 @@ def test_reload(options, expected):
@pytest.mark.parametrize("options, expected", [
(["--umask 0", "myapp:app"], 0),

This comment has been minimized.

@berkerpeksag

berkerpeksag Oct 27, 2017

Collaborator

This should read:

(["--umask", "0", "myapp:app"], 0),
@@ -375,6 +375,8 @@ def test_reload(options, expected):
@pytest.mark.parametrize("options, expected", [
(["--umask 0", "myapp:app"], 0),
(["--umask 0o0", "myapp:app"], 0),

This comment has been minimized.

@berkerpeksag

berkerpeksag Oct 27, 2017

Collaborator

Same as above:

(["--umask", "0o0", "myapp:app"], 0),
(["--umask", "0x0", "myapp:app"], 0),
@hramezani

This comment has been minimized.

Contributor

hramezani commented Oct 28, 2017

@berkerpeksag, thanks, you are right. I fixed the tests

@berkerpeksag berkerpeksag merged commit 90b7dae into benoitc:master Oct 28, 2017

1 of 8 checks passed

couverture-io/py26 Not all changes are covered
Details
couverture-io/py27 Not all changes are covered
Details
couverture-io/py34 Not all changes are covered
Details
couverture-io/py35 Not all changes are covered
Details
couverture-io/py36 Not all changes are covered
Details
couverture-io/py36-dev Not all changes are covered
Details
couverture-io/py37 Not all changes are covered
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@berkerpeksag

This comment has been minimized.

Collaborator

berkerpeksag commented Oct 28, 2017

Thanks!

fofanov pushed a commit to fofanov/gunicorn that referenced this pull request Mar 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment