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

Signature Verification with commits. #1401

Closed
wants to merge 4 commits into from

Conversation

devkhan
Copy link
Contributor

@devkhan devkhan commented Jun 20, 2016

Resolves #1255.

A Verification object inside GitHubCommit.Commit.

To-do:

  • Models
  • Tests
  • Integration tests

@devkhan
Copy link
Contributor Author

devkhan commented Jun 20, 2016

@shiftkey I've added the Verification object inside Commit and not directly under GitHubCommit according to the API.

Also, I don't know whether we should create a class for the signature and payload fields, so I've just marked them as strings for now. Please let me know if they should get their own classes.

@devkhan devkhan force-pushed the sign-verification branch 3 times, most recently from 507b618 to f93f517 Compare June 20, 2016 12:16
@@ -771,7 +771,7 @@ public void GetsCorrectUrl()
client.Get("owner", "name", "reference");

connection.Received()
.Get<GitHubCommit>(Arg.Is<Uri>(u => u.ToString() == "repos/owner/name/commits/reference"));
.Get<GitHubCommit>(Arg.Is<Uri>(u => u.ToString() == "repos/owner/name/commits/reference"), null, AcceptHeaders.SignatureVerificationPreview);
Copy link
Contributor

@ryangribble ryangribble Jun 20, 2016

Choose a reason for hiding this comment

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

looks like a tab/whitespace issue here (and in other places)

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, we don't want to use implementation logic as part of the test, so rather than using the AcceptHeaders helper, can you hardcode the expected header in the tests?

@ryangribble
Copy link
Contributor

@devkhan looks like you need to adjust your tab settings as most files are showing whitespace/alignment issues

@ryangribble
Copy link
Contributor

Also, @Sarmad93 has a PR at #1398 that is aiming to add this verification info to the Commit class as part of the Git Data => Commits client, so you should probably wait nutil #1398 is merged, then the accepts header and verification classes etc will already be there. Then this PR just needs to set the preview header on the Repository commits calls and add the unit and integration tests etc.

@devkhan
Copy link
Contributor Author

devkhan commented Jun 20, 2016

@ryangribble Yeah, I recently started using Xamarin, so didn't noticed the tabs/spaces. Will fix it after #1398 is merged. Thanks.

@shiftkey
Copy link
Member

#1398 has been merged

@devkhan devkhan force-pushed the sign-verification branch from f93f517 to af30260 Compare June 27, 2016 05:52
@ryangribble
Copy link
Contributor

Still got merge conflicts on this one, also dont forget about the tab/space issues 😀

@devkhan devkhan closed this Mar 7, 2017
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.

3 participants