Get commit or tag signature verified flag #738
Conversation
There was a problem hiding this comment.
Thanks for adding this new API @sourabhsparkala ! I hope the changeset will be accepted and released soon.
| @SuppressFBWarnings(value = { "UWF_UNWRITTEN_PUBLIC_OR_PROTECTED_FIELD", "UWF_UNWRITTEN_FIELD", "NP_UNWRITTEN_FIELD" }, | ||
| justification = "JSON API") | ||
| public class GHVerification { | ||
| private String reason, signature, payload; |
There was a problem hiding this comment.
Would it make sense to create a enum which holds all possible reasons? The following page defines possible reasons (please see the "Signature verification object" section):
| private boolean verified; | ||
|
|
||
| /** | ||
| * Gets boolean value. |
There was a problem hiding this comment.
Minor: How about "Indicates whether GitHub considers the signature in this commit to be verified"? (borrowed from https://developer.github.com/v3/repos/commits/)
| } | ||
|
|
||
| /** | ||
| * Gets payload used for the verification. |
There was a problem hiding this comment.
I would suggest "Gets the payload that was signed."
|
Thank you for the review @artem-smotrakov. Please let me know if there are any more changes. |
| INVALID("invalid"), | ||
| VALID("valid"); | ||
|
|
||
| private final String value; |
There was a problem hiding this comment.
Not sure if value is necessary.
There was a problem hiding this comment.
@artem-smotrakov
In general, you'd be correct. But I think the default mapping for enums is all-caps and convert-underscores-to-dashes. Which would result in EXPIRED_KEY converting to expired-key and vice versa. Once tests are added, we can see whether this is needed or not.
| * @author Sourabh Sarvotham Parkala | ||
| */ | ||
| public enum GHReason { | ||
| EXPIRED_KEY("expired_key"), |
There was a problem hiding this comment.
We need at test for each of these values. Or at least a bunch of them. Even if you have to manually create them.
| INVALID("invalid"), | ||
| VALID("valid"); | ||
|
|
||
| private final String value; |
There was a problem hiding this comment.
@artem-smotrakov
In general, you'd be correct. But I think the default mapping for enums is all-caps and convert-underscores-to-dashes. Which would result in EXPIRED_KEY converting to expired-key and vice versa. Once tests are added, we can see whether this is needed or not.
| @@ -584,6 +584,8 @@ public void testCommitShortInfo() throws Exception { | |||
| GHCommit commit = r.getCommit("86a2e245aa6d71d54923655066049d9e21a15f23"); | |||
| assertEquals(commit.getCommitShortInfo().getAuthor().getName(), "Kohsuke Kawaguchi"); | |||
| assertEquals(commit.getCommitShortInfo().getMessage(), "doc"); | |||
| assertFalse(commit.getCommitShortInfo().getVerification().getVerified()); | |||
| assertEquals(commit.getCommitShortInfo().getVerification().getReason(), GHReason.UNSIGNED.get()); | |||
There was a problem hiding this comment.
As noted above, we need tests for at least a few more GHReason values (specifically multi-word values).
|
@sourabhsparkala You see we need some tests right? |
@bitwiseman I am working on the tests now. |
sourabhsparkala
force-pushed
the
sign_verification
branch
from
274a07a
to
b6fe00e
Compare
This fixes hub4j#737 - A new entity GHVerification.java has been added which would be reflecting Verification flag - Updating GHCommit.java and GHTagObject.java with GHVerification - Altering few test cases AppTest.java and GHTagTest.java to verify if the Verification entity is being picked up - A separate test class SignatureVerificationTest.java with the associated wiremock test resources - Adding a new enum GHReason.java - Updating tests to check the GHReason implementation, GHReasonTest.java with the associated wiremock test resources
sourabhsparkala
force-pushed
the
sign_verification
branch
from
b6fe00e
to
a78d2f2
Compare
|
@bitwiseman I have added the test cases as advised. |
|
@bitwiseman Thank you for reviewing and merging the PR. |
This fixes #737
Description