WIP: Adding support for Actions API

[JeffreyPalmer]

See #2078 - Feedback Requested!

Hello! I'm putting this up in order to get some feedback on this initial implementation before I take the time to start polishing and getting ready for a real mergeable PR. Any feedback would be great!

Things still to do:

• I haven't spent any time whatsoever on documentation - I'll clean that type of stuff up once I get some confirmation that we're past the "major changes required" stage.
• There are some tests missing on the Observable side; I'll add those later.
• I haven't really tested this implementation live against the API in anger yet. I see that there are integration tests, but I'm not sure how you'd suggest we approach those.
• There are now 3 different types of Token (AccessToken, RunnerRegistrationToken, and RunnerRemoveToken). This doesn't seem great, since they're all the same. I would have created a generic Token class, but I wasn't sure how the project handles changes to existing APIs (AccessToken already existed). Thoughts?
• I added some missing fields to CheckSuite and updated the tests.

Please let me know your thoughts - I'm happy to make changes as required. As I mentioned, I'm not really a C# developer, so any thoughts on where things can be made more idiomatic, etc., would be appreciated. Thanks!

[codecov]

Codecov Report

Merging #2082 into master will decrease coverage by 1.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2082      +/-   ##
==========================================
- Coverage   67.06%   66.03%   -1.03%     
==========================================
  Files         538      570      +32     
  Lines       14138    14948     +810     
==========================================
+ Hits         9481     9871     +390     
- Misses       4657     5077     +420     

Impacted Files	Coverage Δ
Octokit/Models/Response/WorkflowArtifact.cs	0.00% <0.00%> (ø)
Octokit/Models/Response/Workflow.cs	0.00% <0.00%> (ø)
Octokit/Models/Response/WorkflowJobStep.cs	0.00% <0.00%> (ø)
Octokit/Models/Response/SecretsResponse.cs	44.44% <0.00%> (ø)
Octokit/Models/Response/Secret.cs	0.00% <0.00%> (ø)
Octokit/Clients/ActionsClient.cs	57.14% <0.00%> (ø)
Octokit/Models/Response/WorkflowRun.cs	0.00% <0.00%> (ø)
...ctokit.Reactive/Clients/ObservableActionsClient.cs	0.00% <0.00%> (ø)
Octokit/Models/Response/RunnerRemoveToken.cs	0.00% <0.00%> (ø)
...ctive/Clients/ObservableWorkflowArtifactsClient.cs	0.00% <0.00%> (ø)
... and 22 more

[shiftkey]

@JeffreyPalmer thanks for opening this up, and apologies for the delay in looking it over. Here's some quick thoughts:

I haven't spent any time whatsoever on documentation - I'll clean that type of stuff up once I get some confirmation that we're past the "major changes required" stage.

👍 - if you're feeling like this is too much to tackle in one PR, feel free to focus on a subset of this so we can review, land and publish it sooner. Otherwise happy to proceed as-is.

There are some tests missing on the Observable side; I'll add those later.

👍

I haven't really tested this implementation live against the API in anger yet. I see that there are integration tests, but I'm not sure how you'd suggest we approach those.

I think initially focusing on tests against the read API (so we're not creating things and potentially triggering abuse mechanisms) would be a good start - this will help confirm we're getting information from the API and it's being deserialized correctly. Then we can expand things.

There are now 3 different types of Token (AccessToken, RunnerRegistrationToken, and RunnerRemoveToken). This doesn't seem great, since they're all the same. I would have created a generic Token class, but I wasn't sure how the project handles changes to existing APIs (AccessToken already existed). Thoughts?

I'd be hesitant to merge these unless we're very confident they'll stay the same. Looking at AccessToken for example, it's actually much more than just those two values which aren't deserialized from the API:

https://developer.github.com/v3/apps/#response-5

I'd prefer to enhance that with the additional fields in a follow-up PR. With these new RunnerRegistrationToken and RunnerRemoveToken types, I think it's fine to leave them as-is - who knows how the API might evolve?

I added some missing fields to CheckSuite and updated the tests.

👍

I've skimmed the current code and it all looks good from a code organization perspective (CI is also happy, which is a great sign). Let me know if there's any other questions, otherwise I'm 👍 to proceeding!

[JeffreyPalmer]

Hello!

Unfortunately, a series of events in my life have made it unlikely that I will be able to work on this for the foreseeable future. I hate to dump unfinished code in the project's lap like this, but I can't really change the situation. Sorry about that.

I'm hoping that this starting point will be enough for someone to get this over the finish line. I can probably respond to some questions here and there, if that is of any use at all. I'm hoping that I'll be able to contribute to the project again in the future, once my situation again allows for it.

[shiftkey]

@JeffreyPalmer no problem, and totally understandable.

Gonna close this out as we have the original issue #2078 to discuss it further if anyone else is interested.