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

#6607 - ckeygen should provide a default for the keyfile #11654

Merged
merged 48 commits into from Nov 23, 2022

Conversation

eevelweezel
Copy link
Contributor

Scope and purpose

Fixes #6607

ckeygen doesn't provide a default for the key file, even though a default appears in the prompt, #6607

Contributor Checklist:

This process applies to all pull requests - no matter how small.
Have a look at our developer documentation before submitting your Pull Request.

Below is a non-exhaustive list (as a reminder):

  • The title of the PR should describe the changes and starts with the associated issue number, like “#9782 Remove twisted.news. #1234 Brief description”.
  • A release notes news fragment file was create in src/twisted/newsfragments/ (see: Release notes fragments docs.)
  • The automated tests were updated.
  • Once all checks are green, request a review by leaving a comment that contains exactly the string please review.
    Our bot will trigger the review process, by applying the pending review label
    and requesting a review from the Twisted dev team.

@eevelweezel eevelweezel changed the title 6607 - ckeygen should provide a default for the keyfile #6607 - ckeygen should provide a default for the keyfile Sep 10, 2022
@eevelweezel
Copy link
Contributor Author

needs-review

@adiroiban
Copy link
Member

Thanks for helping with this.

I did just a quick review. Feel free to ignore it and wait for another developer to look over this.

Copy link
Member

@adiroiban adiroiban left a comment

Choose a reason for hiding this comment

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

Thanks for the update.

So I think the main things that need to change

  • Don't mutate the input argument
  • Allow dependency injection
  • Fix the test to have the assertion called

src/twisted/conch/scripts/ckeygen.py Outdated Show resolved Hide resolved
src/twisted/conch/scripts/ckeygen.py Outdated Show resolved Hide resolved
src/twisted/conch/scripts/ckeygen.py Outdated Show resolved Hide resolved
src/twisted/conch/test/test_ckeygen.py Outdated Show resolved Hide resolved
@eevelweezel
Copy link
Contributor Author

please review

@adiroiban
Copy link
Member

I will try to get this reviewed next week, in exchange for your help with the other PRs :)

But if anyone else wants to review it, I am happy to have this merge with a review from another person.

@adiroiban adiroiban dismissed their stale review October 29, 2022 13:37

needs re-review

@glyph
Copy link
Member

glyph commented Nov 23, 2022

Looks like all the feedback was addressed!

@glyph glyph enabled auto-merge November 23, 2022 08:17
@glyph glyph merged commit 49ae3c5 into twisted:trunk Nov 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ckeygen doesn't provide a default for the key file
5 participants