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

new simple example for rcgen #188

Merged
merged 1 commit into from
Nov 22, 2023

Conversation

tbro
Copy link
Contributor

@tbro tbro commented Nov 13, 2023

This takes what was formerly rcgen/src/main.rs and moves it to the examples folder as simple.rs. It was split off from #185

tbro added a commit to tbro/rcgen that referenced this pull request Nov 13, 2023
This is basically rustls#185 minus rustls#188 and rustls#189. The structure also differs
as sub modules have been inlined in `main.rs` and `cert.rs`. `anyhow`
has also been added as a dependency to replace the `Result` alias.

Closes rustls#175
Copy link
Member

@cpu cpu left a comment

Choose a reason for hiding this comment

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

This takes what was formerly rcgen/src/main.rs and moves it to the examples folder as simple.rs

This code is now duplicated instead of moved right (and I think from a different location than described)? It's in rustls-cert-gen/src/main.rs and also rcgen/examples/simple.rs.

Would it make more sense for this to be a git mv rustls-cert-gen/src/main.rs rcgen/examples/simple.rs and then putting an ~empty main.rs in its place?

@@ -0,0 +1,38 @@
#![allow(clippy::complexity, clippy::style, clippy::pedantic)]
Copy link
Member

Choose a reason for hiding this comment

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

I think this could be removed. I only see a handful of clippy findings in this code and they look easy to fix.

rcgen/examples/simple.rs Outdated Show resolved Hide resolved
rcgen/examples/simple.rs Outdated Show resolved Hide resolved
rcgen/examples/simple.rs Outdated Show resolved Hide resolved
rcgen/examples/simple.rs Outdated Show resolved Hide resolved
rcgen/examples/simple.rs Outdated Show resolved Hide resolved
@tbro
Copy link
Contributor Author

tbro commented Nov 13, 2023

This takes what was formerly rcgen/src/main.rs and moves it to the examples folder as simple.rs

This code is now duplicated instead of moved right (and I think from a different location than described)? It's in rustls-cert-gen/src/main.rs and also rcgen/examples/simple.rs.

This was the previous rcgen/src/main.rs. Then it was moved as a placeholder to rustls-cert-gen/src/main.rs.

Would it make more sense for this to be a git mv rustls-cert-gen/src/main.rs rcgen/examples/simple.rs and then putting an ~empty main.rs in its place?

I think the git mv doesn't make much sense because we also have to edit rustls-cert-gen/src/main.rs (add an empty main() to prevent breaking cargo test and such. so it won't end up being a rename in git history anyway.

split off from rustls#185 and make some minor changes.

  * remove inconsistent use of `sys::fs`
  * remove `&` when on file writes
  * remove clippy declaration at top of example
@tbro tbro force-pushed the create-example-from-former-rcgen-main branch from 0d95cb4 to 1f3fd1e Compare November 13, 2023 22:38
@djc
Copy link
Member

djc commented Nov 14, 2023

I think the git mv doesn't make much sense because we also have to edit rustls-cert-gen/src/main.rs (add an empty main() to prevent breaking cargo test and such. so it won't end up being a rename in git history anyway.

I think you can have a rename in the same commit as introducing a new file in the place of the old one? IMO it would be very helpful to make sure this is recorded as a git commit, so that blame and tools like that can see through the move.

@cpu
Copy link
Member

cpu commented Nov 14, 2023

I think you can have a rename in the same commit as introducing a new file in the place of the old one?

That's what I had in mind, sorry - I could have been more explicit.

@tbro
Copy link
Contributor Author

tbro commented Nov 14, 2023

I think you can have a rename in the same commit as introducing a new file in the place of the old one? IMO it would be very helpful to make sure this is recorded as a git commit, so that blame and tools like that can see through the move.

I agree with the intent, unfortunately git wants to outsmart us here. I've tried this and double checked and it does not work. We can achieve what you want with two commits, but I feel there is a good chance those will get squashed/rebased away at some point. If not, just say so and I'll do the two commits.

Here is the best description I could find on git's behavior in this case:
https://stackoverflow.com/a/29822890

@cpu
Copy link
Member

cpu commented Nov 14, 2023

Ah that's a bummer :-( In that case I think what you have here is fine unless someone else has a stronger preference. Thanks for trying!

tbro added a commit to tbro/rcgen that referenced this pull request Nov 14, 2023
This is basically rustls#185 minus rustls#188 and rustls#189. The structure also differs
as sub modules have been inlined in `main.rs` and `cert.rs`. `anyhow`
has also been added as a dependency to replace the `Result` alias.

Closes rustls#175

includes review fixes such as:

  * remove top-level rsa dependency
  * inline parse_san
  * Check for presence of EKU before pushing.
  * Replace `struct Signature` struct w/ `enum KeypairAlgorithm`
  * update some doc strings
tbro added a commit to tbro/rcgen that referenced this pull request Nov 14, 2023
This is basically rustls#185 minus rustls#188 and rustls#189. The structure also differs
as sub modules have been inlined in `main.rs` and `cert.rs`. `anyhow`
has also been added as a dependency to replace the `Result` alias.

Closes rustls#175

includes review fixes such as:

  * remove top-level rsa dependency
  * inline parse_san
  * Check for presence of EKU before pushing.
  * Replace `struct Signature` struct w/ `enum KeypairAlgorithm`
  * update some doc strings
  * make EndEntity and Ca public so they appear in the docs
tbro added a commit to tbro/rcgen that referenced this pull request Nov 14, 2023
This is basically rustls#185 minus rustls#188 and rustls#189. The structure also differs
as sub modules have been inlined in `main.rs` and `cert.rs`. `anyhow`
has also been added as a dependency to replace the `Result` alias.

Closes rustls#175

includes review fixes such as:

  * remove top-level rsa dependency
  * inline parse_san
  * Check for presence of EKU before pushing.
  * Replace `struct Signature` struct w/ `enum KeypairAlgorithm`
  * update some doc strings
  * make EndEntity and Ca public so they appear in the docs
@cpu cpu requested a review from est31 November 15, 2023 18:48
github-merge-queue bot pushed a commit that referenced this pull request Nov 20, 2023
While reviewing #188 I wanted to
confirm that example code was being built in CI. It turns out that it
wasn't. Similarly we haven't been running `clippy` against test code,
and so there was a number of findings to address.

This branch updates CI to:

* Remove `--all`. This is a deprecated alias for `--workspace`, and
`--workspace` is the default for a directory containing a workspace so
it can be omitted.
* Use `--all-targets` whenever we run `cargo check`, `cargo test` or
`cargo clippy`. This ensures coverage for both examples and unit tests.

In order for the `cargo clippy ... --all-targets` to succeed this branch
addresses each of the findings that were present. I've done this with a
separate commit per class of finding to make it easier to review. In one
case (7bfe0ef) I allowed the finding
instead of fixing it since it seemed like the choice of digit groupings
was done intentionally.
@est31 est31 added this pull request to the merge queue Nov 22, 2023
Merged via the queue into rustls:main with commit 2180ada Nov 22, 2023
13 checks passed
tbro added a commit to tbro/rcgen that referenced this pull request Nov 27, 2023
This is basically rustls#185 minus rustls#188 and rustls#189. The structure also differs
as sub modules have been inlined in `main.rs` and `cert.rs`. `anyhow`
has also been added as a dependency to replace the `Result` alias.

Closes rustls#175

includes review fixes such as:

  * remove top-level rsa dependency
  * inline parse_san
  * Check for presence of EKU before pushing.
  * Replace `struct Signature` struct w/ `enum KeypairAlgorithm`
  * update some doc strings
  * make EndEntity and Ca public so they appear in the docs
@tbro tbro deleted the create-example-from-former-rcgen-main branch December 4, 2023 20:10
tbro added a commit to tbro/rcgen that referenced this pull request Dec 4, 2023
This is basically rustls#185 minus rustls#188 and rustls#189. The structure also differs
as sub modules have been inlined in `main.rs` and `cert.rs`. `anyhow`
has also been added as a dependency to replace the `Result` alias.

Closes rustls#175

includes review fixes such as:

  * remove top-level rsa dependency
  * inline parse_san
  * Check for presence of EKU before pushing.
  * Replace `struct Signature` struct w/ `enum KeypairAlgorithm`
  * update some doc strings
  * make EndEntity and Ca public so they appear in the docs
tbro added a commit to tbro/rcgen that referenced this pull request Dec 6, 2023
This is basically rustls#185 minus rustls#188 and rustls#189. The structure also differs
as sub modules have been inlined in `main.rs` and `cert.rs`. `anyhow`
has also been added as a dependency to replace the `Result` alias.

Closes rustls#175

includes review fixes such as:

  * remove top-level rsa dependency
  * inline parse_san
  * Check for presence of EKU before pushing.
  * Replace `struct Signature` struct w/ `enum KeypairAlgorithm`
  * update some doc strings
  * make EndEntity and Ca public so they appear in the docs
  * additional test cases
tbro added a commit to tbro/rcgen that referenced this pull request Dec 11, 2023
This is basically rustls#185 minus rustls#188 and rustls#189. The structure also differs
as sub modules have been inlined in `main.rs` and `cert.rs`. `anyhow`
has also been added as a dependency to replace the `Result` alias.

Closes rustls#175

includes review fixes such as:

  * remove top-level rsa dependency
  * inline parse_san
  * Check for presence of EKU before pushing.
  * Replace `struct Signature` struct w/ `enum KeypairAlgorithm`
  * update some doc strings
  * make EndEntity and Ca public so they appear in the docs
  * additional test cases
github-merge-queue bot pushed a commit that referenced this pull request Dec 20, 2023
This is basically #185 minus #188 and #189. The structure also differs
as sub modules have been inlined in `main.rs` and `cert.rs`. `anyhow`
has also been added as a dependency to replace the `Result` alias.

Closes #175

---------

Co-authored-by: tbro <tbro@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants