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

rust: adds sign_with function #249

Merged
merged 27 commits into from
Mar 6, 2024
Merged

Conversation

tl-flavio-barinas
Copy link
Contributor

@tl-flavio-barinas tl-flavio-barinas commented Feb 13, 2024

BREAKING CHANGE / bumping from v0.1.3 to v0.2.0

  • refactor code to use typed builders.
  • new custom sign option sign_wtih

Once the library is published, I will update the rust examples in a separate PR.

@tl-flavio-barinas tl-flavio-barinas changed the title rust: adds sign with function rust: adds sign_with function Feb 13, 2024
Copy link
Member

@Lindronics Lindronics left a comment

Choose a reason for hiding this comment

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

Nice, I like the typed builder. Just added some nits.

rust/src/sign.rs Outdated Show resolved Hide resolved
rust/src/sign.rs Outdated Show resolved Hide resolved
rust/src/sign.rs Outdated Show resolved Hide resolved
rust/src/sign.rs Outdated Show resolved Hide resolved
rust/src/sign.rs Outdated Show resolved Hide resolved
rust/src/sign.rs Outdated Show resolved Hide resolved
@tl-flavio-barinas tl-flavio-barinas marked this pull request as ready for review February 22, 2024 15:49
@tl-flavio-barinas tl-flavio-barinas requested a review from a team as a code owner February 22, 2024 15:49
Copy link
Member

@Lindronics Lindronics left a comment

Choose a reason for hiding this comment

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

Left some more general comments.

Also the examples need updating.

rust/src/jws.rs Show resolved Hide resolved
rust/src/http.rs Outdated Show resolved Hide resolved
Comment on lines 174 to 186
impl<'a> SignerBuilder<'a, &'a str, Unset, Unset, PhantomData<Get>, &'a str> {
/// Builds a [`CustomSigner`]
pub fn build_custom_signer(self) -> CustomSigner<'a> {
CustomSigner {
kid: self.kid,
body: &[],
method: Get::name(),
path: self.path,
headers: self.headers,
jws_jku: self.jws_jku,
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

If we're getting rid of the old sign_with_pem and verify_with_... functions and going for the full breaking change, do we still need the distinction between old (v1) and new "custom" signers/verifiers?

Just wondering if they can be combined and just expose different functions (depending on which types are populated in the builder).

rust/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@kplattret kplattret left a comment

Choose a reason for hiding this comment

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

Looks good to me, but it would be good to have a second approval for those changes.

rust/CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Kevin Plattret <kevin@plattret.com>
Copy link
Contributor

@tl-marco-tormento tl-marco-tormento left a comment

Choose a reason for hiding this comment

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

lgtm, just some ocd nits

rust/CHANGELOG.md Outdated Show resolved Hide resolved
rust/CHANGELOG.md Outdated Show resolved Hide resolved
rust/CHANGELOG.md Outdated Show resolved Hide resolved
rust/CHANGELOG.md Outdated Show resolved Hide resolved
rust/CHANGELOG.md Outdated Show resolved Hide resolved
rust/src/sign/mod.rs Outdated Show resolved Hide resolved
rust/src/sign/mod.rs Outdated Show resolved Hide resolved
rust/src/sign/mod.rs Outdated Show resolved Hide resolved
rust/src/verify/mod.rs Outdated Show resolved Hide resolved
rust/src/verify/mod.rs Outdated Show resolved Hide resolved
tl-flavio-barinas and others added 4 commits March 5, 2024 18:43
Co-authored-by: Marco Tormento <91872926+tl-marco-tormento@users.noreply.github.com>
Co-authored-by: Marco Tormento <91872926+tl-marco-tormento@users.noreply.github.com>
Co-authored-by: Marco Tormento <91872926+tl-marco-tormento@users.noreply.github.com>
Co-authored-by: Marco Tormento <91872926+tl-marco-tormento@users.noreply.github.com>
tl-flavio-barinas and others added 9 commits March 5, 2024 18:43
Co-authored-by: Marco Tormento <91872926+tl-marco-tormento@users.noreply.github.com>
Co-authored-by: Marco Tormento <91872926+tl-marco-tormento@users.noreply.github.com>
Co-authored-by: Marco Tormento <91872926+tl-marco-tormento@users.noreply.github.com>
Co-authored-by: Marco Tormento <91872926+tl-marco-tormento@users.noreply.github.com>
Co-authored-by: Marco Tormento <91872926+tl-marco-tormento@users.noreply.github.com>
Co-authored-by: Marco Tormento <91872926+tl-marco-tormento@users.noreply.github.com>
Co-authored-by: Marco Tormento <91872926+tl-marco-tormento@users.noreply.github.com>
Co-authored-by: Marco Tormento <91872926+tl-marco-tormento@users.noreply.github.com>
Co-authored-by: Marco Tormento <91872926+tl-marco-tormento@users.noreply.github.com>
Copy link
Contributor

@tl-marco-tormento tl-marco-tormento left a comment

Choose a reason for hiding this comment

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

Lgtm

@tl-flavio-barinas tl-flavio-barinas merged commit 904d429 into main Mar 6, 2024
3 checks passed
@tl-flavio-barinas tl-flavio-barinas deleted the rust_adds_sign_with_fn branch March 6, 2024 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants