feat - add label search argument to sign-invoice #330

VishnuJin · 2022-04-06T13:48:12Z

fixes #299

this PR adds optional ability to sign-invoice with a key that matches the given label

For example

`--label-matching` (partial match)

❯ bindle sign-invoice -o signed-invoice.toml invoice.toml --label-matching "Vishnu Jin"
Signed invoice.toml with role creator, label as 'Vishnu Jin <vishnujin@outlook.com>' and wrote to signed-invoice.toml

`--label` (exact match)

❯ bindle sign-invoice -o signed-invoice.toml invoice.toml --label "Vishnu Jin <vishnujin@outlook.com>"
Signed invoice.toml with role creator, label as 'Vishnu Jin <vishnujin@outlook.com>' and wrote to signed-invoice.toml

thomastaylor312

2 minor changes here.

Also, it would be great if we could have a --label that does an exact match, but that is optional

src/invoice/signature.rs

bin/client/opts.rs

bin/client/main.rs

VishnuJin · 2022-04-07T16:53:06Z

Also, it would be great if we could have a --label that does an exact match, but that is optional

yeah that was my original intention as well.I will implement it

VishnuJin · 2022-04-07T16:53:22Z

@thomastaylor312 can we use the -l as short for --label and -m as short for --label-match ?

thomastaylor312 · 2022-04-07T16:55:51Z

Those sound like great shorthands

thomastaylor312

Awesome! Thanks for going back to make things match. I'll leave this open for a bit in case you want to handle the nit and will merge tomorrow!

bin/client/main.rs

VishnuJin · 2022-04-08T13:19:05Z

src/invoice/signature.rs

    /// In general, if multiple keys match, the implementation chooses the "best fit"
    /// and returns that key.
-    fn get_first_matching(&self, role: &SignatureRole) -> Option<&SecretKeyEntry>;
+    fn get_first_matching(


@thomastaylor312 Just thought it would be better to just have one method in the trait rather than having 2 methods that mostly does the same

VishnuJin · 2022-04-08T13:20:19Z

@thomastaylor312 I have just modified the approach here, if you like this approach I will remove the earlier commits !! let me know your thoughts

thomastaylor312

I actually really like this way of doing it! Just a few comments for discussion

thomastaylor312 · 2022-04-08T17:20:44Z

bin/client/main.rs

+                ClientError::Other(format!("Error loading file {}: {}", keyfile.display(), e))
+            })?;
+
+            let match_type = if sign_opts.label.is_some() {


I really like the enum way of doing this!

One nit here: Probably better to do an if let Some(label) = sign_opts.label.as_ref() instead

Used Match now, as we are matching based on 2 expressions, using if let I felt is a bit strange !!
https://github.com/VishnuJin/bindle/blob/e9e0100b3cca0f5301649f6e5d213fd9aab54f56/bin/client/main.rs#L253-L260

src/invoice/signature.rs

thomastaylor312 · 2022-04-08T17:27:13Z

src/invoice/signature.rs

+    fn get_first_matching(
+        &self,
+        role: &SignatureRole,
+        label_match: LabelMatch,


Would it be better if we did Option<LabelMatch> and drop the LabelMatch::None? That might be a bit cleaner and makes it so people don't have to import LabelMatch if they aren't using it

yeah, I thought its good to have a Default implementation that can be used in test files etc rather than just using None
but let me know if I can change this to Option<LabelMatch > !!

I changed it to Option and removed LabelMatch::None

Discussing new implementation

…hing' to avoid ambiguity

VishnuJin · 2022-04-09T07:30:42Z

@thomastaylor312 this is ready for review now

thomastaylor312 · 2022-04-11T17:21:17Z

src/invoice/signature.rs

    }
 }
+/// This enumerates the select criteria of the key based on the given Label
+pub enum LabelMatch {


Sorry for another comment here. This is entirely optional, but just wanted your opinion as this API will be used relatively often and I was wondering if this could be cleaner:

pub enum LabelMatch<'a> { /// Key will be selected with an exact match of the given label. /// Matches are case sensitive FullMatch(&'a str), /// Key will be selected with a partial match of the given label, /// Matches are case insensitive. /// /// For example: "pika" will match "Pikachu" and "puff" will match "Jigglypuff" PartialMatch(&'a str), }

This would make the enum cheap to construct and no need to copy the String. It also would avoid having an Option<&LabelMatch> in a function signature.

We can also just merge and come back to this later, but if you do have an opinion, I'd love to hear it!

yeah, I had this with lifetime at first but when having it inside first_matching_key function in main.rs I had trouble with the life time reference as expected. Since these string are small I thought its okay to not use reference.
But can I give it another try to have &'a str like before

Nah, no problem. We'll keep this as is and revisit down the line if we want to try again!

bin/client/main.rs

VishnuJin requested review from radu-matei, technosophos and thomastaylor312 as code owners April 6, 2022 13:48

VishnuJin force-pushed the feat-label-search-for-sign-invoice branch from fdadf76 to 2efe16c Compare April 6, 2022 14:01

thomastaylor312 suggested changes Apr 7, 2022

View reviewed changes

src/invoice/signature.rs Outdated Show resolved Hide resolved

bin/client/opts.rs Show resolved Hide resolved

bin/client/main.rs Outdated Show resolved Hide resolved

VishnuJin force-pushed the feat-label-search-for-sign-invoice branch from 2efe16c to c1f102a Compare April 7, 2022 19:45

VishnuJin requested a review from thomastaylor312 April 7, 2022 20:00

thomastaylor312 previously approved these changes Apr 7, 2022

View reviewed changes

bin/client/main.rs Outdated Show resolved Hide resolved

VishnuJin commented Apr 8, 2022

View reviewed changes

thomastaylor312 reviewed Apr 8, 2022

View reviewed changes

VishnuJin force-pushed the feat-label-search-for-sign-invoice branch from b88180a to e9e0100 Compare April 9, 2022 05:32

VishnuJin added 2 commits April 9, 2022 12:40

add label match arguments for sign-invoice

a973a72

minor-update keys print command's '--label' argument to '--label-matc…

ab0b9b2

…hing' to avoid ambiguity

VishnuJin force-pushed the feat-label-search-for-sign-invoice branch from e9e0100 to ab0b9b2 Compare April 9, 2022 07:11

VishnuJin requested a review from thomastaylor312 April 9, 2022 07:31

thomastaylor312 reviewed Apr 11, 2022

View reviewed changes

bin/client/main.rs Show resolved Hide resolved

thomastaylor312 approved these changes Apr 11, 2022

View reviewed changes

thomastaylor312 merged commit a6d246b into deislabs:main Apr 11, 2022

feat - add label search argument to sign-invoice #330

feat - add label search argument to sign-invoice #330

Uh oh!

Conversation

VishnuJin commented Apr 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

For example

--label-matching (partial match)

--label (exact match)

Uh oh!

thomastaylor312 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

VishnuJin commented Apr 7, 2022

Uh oh!

VishnuJin commented Apr 7, 2022

Uh oh!

thomastaylor312 commented Apr 7, 2022

Uh oh!

thomastaylor312 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VishnuJin commented Apr 8, 2022

Uh oh!

thomastaylor312 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VishnuJin commented Apr 9, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

VishnuJin commented Apr 6, 2022 •

edited

Loading

`--label-matching` (partial match)

`--label` (exact match)