Add Actions workflow API clients

[martincostello]

This pull request is the start of implementing support for the GitHub Actions API.

I've tried to structure the code based on existing examples, but there might be places where I've not done the right thing - please point out any such usages.

I've also made assumptions as to the best names to use for the new request and response models where I feel there wasn't a seemingly obviously correct name from looking at the OpenAPI specs.

Contributes to #2078.

TODO

• Top-level Actions client
• Client for Artifacts
• Client for Cache
• Client for Permissions
• Client for Secrets
• Client for Self-hosted runner groups
• Client for Self-hosted runners
• Client for Workflow jobs
• Client for Workflow runs
  • Fix deserialization test for WorkflowRunUsage
  • How to handle that pending-deployment contains an array that can contain User or Team objects?
• Client for Workflows
  • Fix deserialization test for WorkflowUsage
• Integration test with real GitHub APIs

[martincostello]

I've implemented the skeleton of things plus the clients for Workflows and Workflow runs.

I'm going to park this until there's some feedback that I'm on the right track, as this was more involved than I expected due to to the various conventions (like having to also mirror things for the Reactive version).

[JonruAlveus]

Wow nice! This very much looks like you’re on the right tracks. Just a couple of thoughts

• You don’t need to worry about accept headers, we’re just in the process of removing them (mostly)
• With enum there was a move at some point to move towards a ‘string enum’ class. I think it was to handle cases where a value was added to the api and would cause the code here to explode if it was a plain enum. I’m not sure if you need it here but might be worth thinking about
• I know it’s early but some integration tests might help. Have a look at RepositoryClientTests for some good examples

Looks like some great work. Let me know if I can help in any way ❤️

[martincostello]

Thanks for the feedback @JonruAlveus! I'll start making further progress on this in the next week or so now I know I'm not going down the wrong path entirely.

[martincostello]

It might not be the right place to put the fix, but I discovered the issue with deserializing the workflow usage which has ALLCAPS fields was due to the ToRubyCase() method changing the overridden key name from the [Parameter] attribute. I added a special case for that in f8ed1c4, but it might be better to just return the overridden value here instead of letting it go through ToRubyCase():

octokit.net/Octokit/Helpers/ReflectionExtensions.cs

Lines 17 to 22 in 80b82aa

	if (paramAttr != null && !string.IsNullOrEmpty(paramAttr.Key))
	{
	memberName = paramAttr.Key;
	}
	return memberName.ToRubyCase();

[kfcampbell]

@martincostello is this something you're still interested in working on? Perhaps it might make sense to split the large amount of functionality being introduced into smaller PRs? We can provide some assistance to get this in if you'd like to continue.

[martincostello]

That would be a good idea if we could break it up into chunks and then go from there.

The size of the Actions API has made this quite a daunting prospect that I haven’t had the time or headspace to come back to tackle 😅

I’d definitely like to get this moving again though.

[kfcampbell]

👍 Awesome! The size of the API is pretty big, definitely. Why not start with the top-level Actions client and the client for workflow runs as you've specified?

[martincostello]

Right I've got to the point where I think the client for Actions workflows is essentially complete.

I just have these final queries, then I can address them and bring this out of draft.

1. pending-deployment contains an array that can contain User or Team objects - how should this discriminated union be represented in the .NET models?
2. For the clients I'm not implementing as part of this PR, what should I do about the skeletons I've added so far? Possibilities include:
   • Deleting them completely;
   • Making them internal so they can be made public at a later date when they're fleshed out.
3. Adding new properties to the existing interfaces makes this a SemVer breaking change (and so will adding the other clients later). Do I need to do anything to bump the version in this PR, and will (potentially lots) of subsequent major version bumps to add the rest of the clients be acceptable?

[martincostello]

There's another question for this PR now in addition to the outstanding ones above.

With the merging of #2598, there might be a conflict between how that PR has structured Actions (children of repositories and organisations) vs. how I structured this PR (all Actions APIs are children of a new top-level resource).

Do I need to change the structure of this PR in light of that change?

[nickfloyd]

Do I need to change the structure of this PR in light of that change?

Hey @martincostello, good catch. Apologies for the delay in getting back to you.

I favor how you've done it here by keeping Actions free of the knowledge of Orgs as a top-level concern. Your approach isolates the concern of Actions rather than them being directly coupled to an org or repo (even though that relationship is technically correct) - you've just broken those relationships out, which feels, to me, more clean.

The design you propose here leads to reduced chattiness, greater flexibility, and resource isolation. Once this draft is synced with main, would you mind giving your efforts here a once over to ensure that artifacts of the old pattern have been removed? I noticed that you've already done that with a few things.

[martincostello]

Sure, will do. I'll try and get this cleaned up and out of draft this week.

[martincostello]

This is all synced with main now, and I think complete for workflow jobs and runs modulo the question in the description and the question about breaking changes #2429 (comment).

How to handle that pending-deployment contains an array that can contain User or Team objects?

[kfcampbell]

I'm honestly not sure what the best way to handle that is. You could use an array of Object/dynamic and cast, or an ArrayList that can hold multiple object types and cast, or you could try to make a base class that both inherit from. I don't love any of the options.

[martincostello]

I'll have a think about how to do the base class option.

[nickfloyd]

How to handle that pending-deployment contains an array that can contain User or Team objects?

So here are my 2 cents on this....

It feels like there are 2 different problems at play here:

• The polymorphism question - having a generic List with typed objects - I feel like that can be solved with Interfaces or Inheritance (see my prototype code below)
• The hydration question. Having an inferred type from an actual type node is problematic for both generative and automatic hydration logic.

NOTE: We'd have to come up with a shim in the automatic serialization that looks for that type of property and then casts the object as that type. here and here.

The main stumbling block is around the use of a type node where there is a collection of resources (reviewers) that is polymorphic (containing Users and Teams). This specific design makes it difficult for generative aspects when using OpenAPI as well as makes things more challanging when it comes to hydrating concrete types.

I've reached out to the owning team to see if I can get some clarity on the design decision here before we take steps to over engineer something that will just end up being tech debt to cover a unique case. There might be other reasons this needed to be done this way that I am missing and I don't want to overlook that.

---

Example of base class and or Interface (either should work with generics)

class PendingDeployments {
  ...
  // Pending deployment props
  public List<Reviewer> Reviewers {...}
}

public interface IReviewer {...}

class Reviewer : IReviewer {
  ...

  public int Id {...}
  public string NodeId {...}
  public string url {...}

}

class User : Reviewer, IReviewer {
  // User specific props
  ...
}

class Team : Reviewer, IReviewer {
  //Team specific props
  ...
}

[martincostello]

Thanks for the detailed response. I'll wait on a reply for your queries to the other team before picking this back up.

[nickfloyd]

So, I'm currently chatting with the team. The data model seems pretty flat here - with a join table in between - which is why this is represented like it is. Not ideal, but I do understand the constraint.

So we have a couple of options:

1. We sit on this changeset (I don't like that idea - @martincostello has done so much good stuff in here) until we get a change rolled into the REST API to have a more concrete implementation - something like:

"reviewers" : {
  "users" : [
    {...}, {...}
  ]
  
  "teams": [
    {...}, {...}
  ]
}

2. We write a wedge/middleware and produce the representation we want. While this would get us where we need to be and help us with a hopeful future, it would add debt - so I'm not too fond of that either.

Give me a bit more time to see what we can do on REST API side - perhaps we could get them to add a new field that has the collections mapped to their respective resource representations. Thanks for the work and your patience on this @martincostello

[martincostello]

I've pushed up a rebase with a commit that removes the pending deployments.

Alternatively, I can put them back but just completely omit the reviewers from the PendingDeployment response so the endpoint is still usable as long as you don't care about who could do the approvals.

[nickfloyd]

I've pushed up a rebase with a commit that removes the pending deployments.

Alternatively, I can put them back but just completely omit the reviewers from the PendingDeployment response so the endpoint is still usable as long as you don't care about who could do the approvals.

Let's leave it as you have it. 🥇 for all of the great test coverage as well! So good! I'm reviewing this now and will hopefully merge and release it today. Thank you so much for the effort that you've put in here to make this SDK and the community better!

[nickfloyd]

LGTM ❤️