Skip to content

Use UTF-8 encoding for nginx plugin#7797

Merged
schoen merged 17 commits intocertbot:masterfrom
g6123:nginx-utf8
Mar 14, 2020
Merged

Use UTF-8 encoding for nginx plugin#7797
schoen merged 17 commits intocertbot:masterfrom
g6123:nginx-utf8

Conversation

@g6123
Copy link
Copy Markdown
Contributor

@g6123 g6123 commented Feb 23, 2020 •
edited
Loading

Based on and supersedes #6725, which was originally based on #5341.

Fixes #5337.
Closes #6725.

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.

@g6123
Copy link
Copy Markdown
Contributor Author

g6123 commented Feb 23, 2020 •
edited
Loading

@ohemorange Hi, it has been a long time. I've checked @schoen's PR, and sadly it was yet far away from recent master branch and has been updated too long ago. So I made a new one, to finally fix this old issue.

@g6123
Copy link
Copy Markdown
Contributor Author

g6123 commented Feb 23, 2020 •
edited
Loading

Test is failing in 2.7. It is because of TestCase.assertLogs(), which was introduced in Python 3.4. Should I just remove this logging test, or back-port the feature?

certbot/certbot-nginx/tests/parser_test.py

Line 493 in d68f37a

with self.assertLogs() as log:

certbot/certbot-nginx/tests/parser_test.py

Line 513 in d68f37a

with self.assertLogs() as log:

@g6123
Copy link
Copy Markdown
Contributor Author

g6123 commented Feb 23, 2020 •
edited
Loading

Back-porting shouldn't be that hard, since there is already an implementation in CPython:

@g6123 g6123 requested a review from ohemorange February 23, 2020 18:28
@g6123
Copy link
Copy Markdown
Contributor Author

g6123 commented Feb 23, 2020

Okay, I've added TestCase.assertLogs() backport implementation to nginx TC.

schoen
schoen reviewed Feb 25, 2020
View reviewed changes
schoen
schoen suggested changes Feb 25, 2020
View reviewed changes
@schoen
Copy link
Copy Markdown
Contributor

schoen commented Feb 25, 2020

Thanks, I added a few comment on this.

@g6123 g6123 requested a review from schoen February 25, 2020 04:27
@g6123
Copy link
Copy Markdown
Contributor Author

g6123 commented Feb 25, 2020 •
edited
Loading

Thank you @schoen ! I've added some changes.

schoen
schoen previously approved these changes Mar 13, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@schoen schoen left a comment

Choose a reason for hiding this comment

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

Thanks a lot @g6123! In addition to looking over the code, I just tested this out on a server and confirmed that it skipped over a configuration file with non-UTF-8 encodings (printing a useful warning on the console). I think this is ready to go.

@schoen schoen merged commit 30ec4ca into certbot:master Mar 14, 2020
@alexzorin alexzorin mentioned this pull request Jul 16, 2020
Closed
@alexzorin alexzorin mentioned this pull request Nov 26, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ohemorange ohemorange Awaiting requested review from ohemorange

1 more reviewer

@schoen schoen schoen approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Nginx plugin crashes when non-ascii configuration file read

2 participants

@g6123 @schoen