Migrations API support

[devkhan]

Refer #1029 .

Migrations API: https://developer.github.com/v3/migration/migrations/

About migrations: https://help.github.com/enterprise/2.4/admin/guides/migrations/about-migrations/

To-do:

• Add models for Migrations request and response.
• Add IEnterpriseMigrationsClient.
• Implement IEnterpriseMigrationsClient.
• Implement Reactive client.
• Re-order methods.
• Refactor (Naming methods and file organization).
• Documentation.
• Unit tests (in progress, help needed).
• Integration tests (I don't have access to an Enterprise instance, some pointers needed).
• Run .\build.cmd FixProjects (for Mono and Xamarin projects).
• Final wiring of parts.
• Find someone who will actually use it. (otherwise its just pointless).

I also need to discuss some of things which pop-up in my head every now and then. 🙏

/cc @ryangribble @shiftkey

[devkhan]

@ryangribble The build is failing for other projects, do I need add all the files there too?

[ryangribble]

Yeah, you have to run the build script locally, with "FixProjects" target - this adds any new files added to octokit project, to the other mono/core/etc projects

Instructions in the contributors doc

[ryangribble]

So I was reading the documentation about this feature and it turns out I was incorrect in saying you need github enterprise for this API

These API calls cover the process of extracting repositories from github.com or github enterprise, that can later be imported into github enterprise. The API is in preview mode only and on covers the export side, there doesn't appear to be anything on import side yet.

Also this preview API isn't yet available on github enterprise as of latest 2.5 version (according to docs at least) so at the moment it seems only possible to do against github.com (usually new features make it into the next ghe feature release so I'd expect to see them in 2.6, potentially with the import side implemented as well or possibly not)

Good news is this means you should be able to do all the testing and integration tests etc yourself @devkhan

[devkhan]

Thanks @ryangribble, I will try my best. And I'd would still need your and others' help.

[devkhan]

@ryangribble @shiftkey Can anybody tell me why CanBeDeserialized() test is failing?

Stack trace:

It fails here:

[ryangribble]

Hey @devkhan

Great job getting a good head start on this 😀 Implementing a whole new "client" is a pretty maojr job, so there are quite a few things we'll need to cover to get you over the line! We really appreciate your contribution so please don't be put off by the following feedback, I'm trying to make it as detailed and explanatory as possible so that we can quickly get you into the "mindset" of an octokit developer 🚀

API Client Structure

We try to structure the API clients in octokit to be inline with the GitHub API. If you have a look at the sidebar in the API docs, the Migrations client lives under a menu item called Migration and does not belong under the Enterprise API.
This means, you actually need to create a "IMigrationClient" that lives in IGitHubClient, then a IMigrationClient that lives in MigrationsClient. Yes this is confusing since there is Migration->Migrations and also confusing since the docs refer to it as the Enterprise Migration API, but we live and die by the API structure in the sidebar, therefore when using Octokit we should match that structure. eg client.Migrations.Migration.Start()

As an aside, it looks like this Migration/Migrations API actually got moved to this top level location late February, previously it was under the Organizations API. I see there is also a new Source Import API under Migration that I will raise an issue for shortly!

Naming things (TM)

If you have a look at some other Client APIs for inspiration, you'll see there are some naming conventions in Octokit which should be consistent. For example, since it's the Migrations API, you wouldnt call the List Migrations method GetMigrations() (since that would be client.Migration.Migrations.GetMigrations() !!! ). We normally name the "list" methods Get() (for a single instance) GetAll() (for all instances) and GetAllForCurrent() (for all instances for the current user, when it's something a user can own, like repositories, publickeys etc. So i'd suggest the names for the methods in this API should be
Start a migration: Start()
List of migrations: GetAll()
Status of migration: Get()
Download migration archive: GetArchive()
Delete migration archive: DeleteArchive()
Unlock repository: UnlockRepository()

Order of Methods

In a similar line of thought, I personally try to have the "order" of the method calls in my client, match those shown in the API. So in your case, you've listed them as
Status, List, Start, Download, Delete, Unlock
whereas the API docs list them in a slightly different order
Start, List, Status, Download, Delete, Unlock
Personally I'd prefer to match the order to the API docs as it makes it easier to review and check for completeness etc 😀

Other Notes

• Dont forget to set the custom accepts header to enable this preview functionality, otherwise it wont actually work when you do it "for real". Have a look at the AcceptHeaders.ProtectedBranchesApiPreview as an example on how to do this. Basically you will add a new entry into that AcceptHeaders helper class, then on your API methods you will need to pass that accept header to the ApiConnection.Post/Get/Delete methods
  

• With your serialization problem, the tiniest little things can sometimes cause so much trouble!!!
  If you have a look at all other Response model objects, you will notice they have a default/parameterless constructor! That's all your problem boils down to... 1 line! haha 😭

  Internally the SimpleJsonSerializer builds up that ConstructorCache and in your Migration class, it can't find a constructor to use, since you only provided one that takes all the parameters! Adding a default constructor should get you past this hurdle!
  public Migration() { }. Same issue will be with your StartMigrationRequest request object. If you want to have a unit test that tests the deserialization, it will need a default ctor

[devkhan]

@ryangribble Thanks for the detailed feedback. I was also eagerly waiting for someone to get me into the "mindset". Now that I know much better about the philosophy of Octokit, I'll adhere to it.

... Migrations client lives under a menu item called Migration and does not belong under the Enterprise API.
This means, you actually need to create a "IMigrationClient" that lives in IGitHubClient, then a IMigrationClient that lives in MigrationsClient...

Thankfully, it came up before running .\build.cmd FixProjects
I know it my fault that I didn't first checked the docs, but since you said an Enterprise instance is needed for this, I just assumed it is a part of Enterprise API. I'll keep this in mind in the future.

Also, a question, does it mean we have to create an IMigrationsClient and an ISourceImportClient (may be in the future) inside an IMigrationClient and that will go under the IGitHubClient?

Naming things (TM) and Order of methods

I was having a hard time Naming Things (TM), its now cleared now, and I'll follow the conventions from now on. 👍

---

And, I was about to ask where to put the AcceptHeader, thanks for clearing that out.

If you have a look at all other Response model objects, you will notice they have a default/parameterless constructor! That's all your problem boils down to... 1 line! haha 😭

That was such a nightmare. Thank you so much. 💯

[ryangribble]

Thank god, it came up before running .\build.cmd FixProjects

Indeed! We have an open issue #1092 suggesting to make "FixProjects" build task remove items from the slave projects, if the items no longer exist on disk... we've all "been there" when we rename something and have to go hacking around in those other projects to set the world right again. Unfortunately I am too chicken to tackle the F# build script to see what's involved in doing that!

Also, a question, does it mean we have to create an IMigrationsClient and an ISourceImportClient (may be in the future) inside an IMigrationClient and that will go under the IGitHubClient?

Yep! I think this is the first time they've given us 2 such similarly named things as parent/child, but the structure is a "Migration" entry, with "Migrations" and "Source Import" under that. So although it's a bit odd to have client.Migrations.Migration.GetAll() etc, that's what they've come up with, so that's what we'll implement 😬

[devkhan]

@ryangribble The GitHub API docs list the Migration->Migrations API as the "Enterprise Migrations API", but does not list it under the Enterprise section, so should I add into a new section for Migration or let it stay in the Enterprise section?

[ryangribble]

Yeah, I mentioned earlier it is confusing since they doi call it the "Enterprise Migrations API" but with octokit.net, we base the Clients class structure on the API structure (as shown in the sidebar on the doc site) and not the name... so GitHubClient.Migration.Migrations is what you should go with (and similarly move all the models, tests etc out of the Enterprise folders)

[ryangribble]

Also from a consistency point of view, in the Client interfaces and implementations you've got method parameters on separate lines

Task<Migration> Start(
   string org,
   StartMigrationRequest migration);

Whereas to be consistent with the existing Clients in the code base, they should be

Task<Migration> Start(string org, StartMigrationRequest migration);

[devkhan]

Thanks. As the GitHub API docs says.

And, I just put them on the next line to improve readability because of long parameters list. But project's conventions are more important, I'll change them. Should I change this in the tests also?

[ryangribble]

indentation

[devkhan]

👍

[ryangribble]

Sorry, to clarify I'm saying the indentation here doesn't look correct...

[devkhan]

I know, I just copy-pasted from the docs, I'll correct them.

[ryangribble]

Should I change this in the tests also?

I was only referring to the method declarations, and the test methods dont take any parameters so it;s not applicable to those...

[devkhan]

@ryangribble The GitHub API docs refer the Migrations API as the "Enterprise Migrations API", so while I will keep it out of the enterprise directory, should I prefix it with Enterprise or not as ...

we live and die by the API structure in the sidebar, therefore when using Octokit we should match that structure.

So, what should I do?

[ryangribble]

So, what should I do?

The mentions of "Enterprise Migrations" is only at the landing page for the parent "Migration" page. Everywhere else shows "Migrations" - eg the URL, the sidebar, the actual page title etc. This seems to be the first example I can find where the "title" of an API as shown on it's parent's landing page is not consistent with it's name in the side bar and on it's own page!

This API was already only recently moved/relocated to this spot, so I assume this is it's new home now, and if anything it may be losing the "Enterprise" nomenclature... Perhaps @shiftkey or @haacked can get the inside word on whether this is being renamed "to" or "away" from "Enterprise" Migrations API on it's parent page?

This is the only place it's called "Enterprise Migrations"

Everywhere else it's just "Migrations"

I think your latest revision is great though - IMigrationClient being the top level parent, and IMigrationsClient being the lower level migrations API. The amusing thing is that due to our policy in octokit of naming the API client property in the singular, we still end up with client.Migration.Migration.GetAll() 😭

But I do think the way you've got it now is the most consistent and inline with the standards set throughout octokit (rather than breaking the singular property naming rule or calling it EnterpriseMigration eventhough its not called that in most places in the doc)... so Im happy with how it's ended up!

[ryangribble]

This PR is looking great now!!! Great effort on the testing coverage and everything is looking nicely consistent and thorough! 👍

I think there's just the one indentation issue with the json in the model test... and the fact that other PR merges have caused you to be out of date with master which you'll need to rebase or merge (sorry!)

Ill pull this down and run through the tests etc myself in the next couple of days

[devkhan]

@ryangribble I still have to write the integration tests, though. I'll try to get them done as soon as possible.

[devkhan]

@ryangribble I've added the integration tests but I have a couple of questions:

• The GetArchive() test is failing. Maybe it is returning a string and I'm doing something wrong. Can you please look into it? Here is the API method, see if needs modification.
• The DELETE based end-points throws "Accept header error", but how do we specify the preview header in the DELETE methods, all the other preview API's also doesn't specify it in their DELETE method.

[ryangribble]

@devkhan ill pull your stuff down and have a look through it in the next couple of days

thanks!

[ryangribble]

This should reflect the method name which was renamed, so TheStartMethod()

[devkhan]

Yeah, another PR has been merged since my rebase. Fixed.

[ryangribble]

I'm going to merge this in as it's been a long hard slog for you @devkhan!

Good work on implementing a brand new API from scratch!

[devkhan]

@ryangribble @shiftkey

It was a great experience, I hope to contribute more in future.
Thanks a 💯 to both of you. It wouldn't have been possible without you.

[shiftkey]