-
Notifications
You must be signed in to change notification settings - Fork 22
#163 Update for josepy 2 #164
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #164 +/- ##
===========================================
+ Coverage 50.00% 62.69% +12.69%
===========================================
Files 17 15 -2
Lines 1042 957 -85
Branches 146 63 -83
===========================================
+ Hits 521 600 +79
+ Misses 508 339 -169
- Partials 13 18 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@glyph Most probably you don't have time for this project...and I think that you are using certbot directly. I am using this code and it works for me. This PR updates the certbot dependencies. I will merge this code. I am looking to get a txacme v2 public release. I am removing the eliot code and convert to inline callbacks. It's easier to debug the code. The testing suite now uses the public Let's Encrypt staging server and this PR also adds the usage of pebble server to help with end to end testing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates the txacme library to work with modern versions of the acme and josepy libraries (acme v4 and josepy v2) while removing the dependency on PyOpenSSL. The changes include significant refactoring of the client code, removal of deprecated features, and updates to testing infrastructure.
Key changes:
- Updated dependencies to acme>=4.0.0 and josepy>=2, removed PyOpenSSL dependency
- Refactored Client class to use new ACME v2 API patterns and simplified initialization
- Removed libcloud DNS responder functionality and integration tests
- Added new Pebble-based integration tests with GitHub Actions workflow updates
Reviewed Changes
Copilot reviewed 15 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/txacme/client.py | Major refactoring of Client and JWSClient classes to work with acme v4 API |
| src/txacme/test/test_client.py | Updated test constants and added new Pebble integration tests |
| src/txacme/service.py | Removed deprecated backend parameter from certificate loading |
| src/txacme/messages.py | Removed custom CertificateRequest class (now using acme library version) |
| src/txacme/challenges/ | Removed libcloud DNS responder support |
| setup.py | Updated dependencies and Python version requirements |
| .github/workflows/test.yaml | Updated CI to use newer Ubuntu, Python versions, and Pebble service |
| content_type = attr.ib(default=JSON_CONTENT_TYPE) | ||
| nonce = attr.ib(default=None) | ||
| json = attr.ib(default=lambda: succeed({})) | ||
| json = attr.ib(default=lambda: defer.succeed({})) |
Copilot
AI
Aug 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lambda function is created for every instance of TestResponse. Consider using a module-level function or constant to avoid repeated lambda creation.
| json = attr.ib(default=lambda: defer.succeed({})) | |
| json = attr.ib(default=_default_json) |
|
@glyph as I don't think that I can get a review from a human for this PR, I am using AI as a fallback... |
…Let's encrypt. Update docstring.
|
I would rather archive the project than start allowing AI reviews to approve merges anywhere. (I am surprised that copilot was allowed on this repo; I have done my best to disable and ban it everywhere) |
|
I think if you want to force a merge due to lack of reviewer energy, I would say that's OK. No need to use Copilot to generate incorrect suggestions first. But this PR was never submitted for review, so it was never on anybody's dashboard. Let's at least request a review from twisted contributors before hitting the big red button in the future. |
|
Sorry for the trouble. I will requests reviews for the changes from here. I though that txacme is dead and nobody cares about it. I will try to find some time to write more end to end tests using the I don't think that you can stop Copilot... you can stop Gemini and other external bots... but Copilot is now a "core" part of Github. |
No worries.
If we can't get reviews we can always start to make exceptions and change the process. But we should always try first :).
That would be great to have in CI, given that this is a project which is implicitly a live-service client. I look forward to seeing those!
Yeah I don't see anything in any of the various toggles that would have prevented this. |
|
It doesn't look like you can block the bot, either. |
Fixes #163
This updates the code to work with latest acme and josepy and removes depenency on PyOpenSSL
It also contains a few cleanups
pypy was removed as certbot no longer supports it since acme v3 ... and this code was updated to use acme 4