-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[FEAT]: Create a GitHub App from a manifest #2939
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for contributing! I had a few (hopefully quick) questions.
@@ -31,57 +32,62 @@ public GitHubApp(long id, string slug, string name, User owner, string descripti | |||
/// <summary> | |||
/// The Id of the GitHub App. | |||
/// </summary> | |||
public long Id { get; private set; } | |||
public long Id { get; protected set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for (and the ramifications of) the permissions change here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To support inheritance.
GitHubAppFromManifest
inherits from GitHubApp
. The SimpleJson
/SimpleJsonSerializer
this project uses won't populate any of the properties inherited from GitHubApp
on GitHubAppFromManifest
objects if the property is private set;
(i.e. id
, slug
, name
, etc. are all null
).
Another example is License
subclasses LicenseMetadata
. All properties on LicensesMetadata
are protected set
As far as ramifications, I can't think of any that would be negative. From the docs:
Only code in the same class or in a derived class can access this type or member.
Honestly, it would be a big help if the standard in this library was to use protected set
instead of private set
. I've often tried to create subclasses of types from this library ( likeRepository
or GitHubApp
) to quickly add and deserialize properties being returned by the API but missing from this library's types. Unfortunately, this is not possible due to the issue above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably one of the best arguments for using protected
that I have heard so far. Just for some context on why they are all private, you can read this issue and see this PR and release if you're interested.
The gist was that the library was pretty inconsistent when it came to how things were implemented and even a couple of years ago we knew we wanted to head to a generative approach. We went with the more restrictive pattern of using private
accessors as a clean slate starting point so that we'd have some consistently and so that we could generate source using that consistency.
Given that we are close to GA'ing our Generated .NET SDK and for the points you made, it makes sense to make mutating these properties more permissive.
Please feel free to push forward on that front if you have time. As a word of caution, the custom serializer that has been built makes use of property accessors and you might run into a couple issues around the serialization tests - but they should be relatively low risk.
/// Represents a GitHub application from a manifest. | ||
/// https://docs.github.com/rest/apps/apps#create-a-github-app-from-a-manifest | ||
/// </summary> | ||
[DebuggerDisplay("{DebuggerDisplay,nq}")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When creating a new GitHub App using the Manifest flow, GitHub redirects the user to the external API with a code
query parameter that the service can exchange for all the details about the new GitHub App, including the Client Id, Client Secret, auto-generated Webhook Secret, PEM, etc.
As such, it's critical to deserialize the additional values on this type and save them for the new GitHub App's backing service to use.
From GitHub's official REST API spec:
"/app-manifests/{code}/conversions":
post:
summary: Create a GitHub App from a manifest
description: Use this endpoint to complete the handshake necessary when implementing
the [GitHub App Manifest flow](https://docs.github.com/apps/building-github-apps/creating-github-apps-from-a-manifest/).
When you create a GitHub App with the manifest flow, you receive a temporary
`code` used to retrieve the GitHub App's `id`, `pem` (private key), and `webhook_secret`.
tags:
- apps
operationId: apps/create-from-manifest
externalDocs:
description: API method documentation
url: https://docs.github.com/rest/apps/apps#create-a-github-app-from-a-manifest
parameters:
- name: code
in: path
required: true
schema:
type: string
responses:
'201':
description: Response
content:
application/json:
schema:
allOf:
- "$ref": "#/components/schemas/integration"
- type: object
properties:
client_id:
type: string
client_secret:
type: string
webhook_secret:
type: string
nullable: true
pem:
type: string
required:
- client_id
- client_secret
- webhook_secret
- pem
additionalProperties: true
examples:
default:
"$ref": "#/components/examples/integration-from-manifest"
'404':
"$ref": "#/components/responses/not_found"
'422':
"$ref": "#/components/responses/validation_failed_simple"
Before the change?
After the change?
repository_custom_properties
organization_copilot_seat_management
organization_custom_properties
Pull request checklist
Does this introduce a breaking change?
Please see our docs on breaking changes to help!