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

Implement GitHub Enterprise Search Indexing API #1095

Merged
merged 3 commits into from
Feb 11, 2016

Conversation

ryangribble
Copy link
Contributor

Status

🚀 Ready to merge!

This is built on top of #1073 which is not yet merged. Only the top 3 commits are actually relating to this PR. Once #1073 is merged, I will rebase this PR.

Features Implemented

Implement Enterprise Search Indexing Client

https://developer.github.com/v3/enterprise/search_indexing/
Fixes #1026
Includes normal and reactive implementations plus unit and integration tests for both

Test Notes

The integration tests require a GitHub Enterprise instance so wont run as part of Octokit builds currently. Integration tests have been run succesfully against a local GHE instance

@ryangribble ryangribble changed the title Enterprise searchindex api Implement GitHub Enterprise Search Indexing API Feb 5, 2016
@ryangribble
Copy link
Contributor Author

@shiftkey Linux build failure looks unrelated. It's the "type initializer for list threw exception" variant

@ryangribble
Copy link
Contributor Author

Could you guys kick off the travis build again when you have a sec? Failure was unrelated

@shiftkey
Copy link
Member

shiftkey commented Feb 8, 2016

@ryangribble 👢ed and 👀ing

@ryangribble
Copy link
Contributor Author

Dont forget that #1073 needs to go first, then this one, then #1022 😀

@shiftkey
Copy link
Member

shiftkey commented Feb 9, 2016

@ryangribble thanks for the heads-up, I'll make sure to prioritize these reviews before any other Octokit stuff as I know merging csproj changes is all kinds of terrible right now 😁

@shiftkey
Copy link
Member

@ryangribble #1073 has been merged 🤘

@ryangribble ryangribble force-pushed the enterprise-searchindex-api branch from 6d9b5f4 to b3049ee Compare February 11, 2016 05:08
@ryangribble
Copy link
Contributor Author

Awesome, this has now been rebased on master and should be ready for review and merge

@ryangribble
Copy link
Contributor Author

@shiftkey builds passed, ready when you are :)

@haacked
Copy link
Contributor

haacked commented Feb 11, 2016

✨ Flawless Victory! ✨

This looks great! The only comment I have is that if we add new targets, we'll have to add more methods to this class. We could consider adding a method that takes in a raw target string, but I don't think we need to do that now. Maybe this API very rarely changes.

haacked added a commit that referenced this pull request Feb 11, 2016
Implement GitHub Enterprise Search Indexing API
@haacked haacked merged commit 660df9d into octokit:master Feb 11, 2016
@ryangribble ryangribble deleted the enterprise-searchindex-api branch February 11, 2016 23:30
@ryangribble
Copy link
Contributor Author

Yeah we had a bit of discussion on the issue #1026 about which way to go and ended up with the multiple methods. Thanks for merging!

@haacked
Copy link
Contributor

haacked commented Feb 11, 2016

Yeah we had a bit of discussion on the issue #1026 about which way to go and ended up with the multiple methods.

Ah, cool. Yeah, I like it. I'm just thinking about adding one additional method as a failsafe. But I don't want to do that yet.

Thanks for merging!

Thanks for contributing!

@ryangribble
Copy link
Contributor Author

Ah, cool. Yeah, I like it. I'm just thinking about adding one additional method as a failsafe. But I don't want to do that yet.

What would the failsafe be, out of interest?

Thanks for contributing!
It's a pleasure on such a weel organised dotnet OSS project. My goal is to implement all the GitHub Enterprise API's currently unsupported. I think im just down to the Setup Console now (which is pretty big) as well as some extra methods on User Administration that are enterprise only.

eg normal API doco for User Administration:
https://developer.github.com/v3/users/administration/

vs Enterprise API doco for User Administration
https://developer.github.com/enterprise/2.5/v3/users/administration/

Heaps more nice methods to create/delete users, impersonation tokens, etc. Fun times 😀

@haacked
Copy link
Contributor

haacked commented Feb 11, 2016

What would the failsafe be, out of interest?

A method that took a target request object (or string). That way, if Enterprise ever added a new target string syntax, you wouldn't have to wait for the next Octokit.net to start calling it.

@ryangribble
Copy link
Contributor Author

Nice idea, I'll add it in the next couple of days!

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