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.

Sign up for GitHub

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

Jump to bottom

Add missing properties for meta and app payloads #2625

Merged
merged 5 commits into from
Dec 2, 2022
Merged

Add missing properties for meta and app payloads #2625

merged 5 commits into from
Dec 2, 2022

Conversation

MatisseHack
Copy link
Contributor

@MatisseHack MatisseHack commented Nov 30, 2022 •
edited
Loading

Resolves #2624

Behavior

Before the change?

  • InstalledVersion was missing from the Meta model (only present on GHES)
  • Permissions and Events were missing from the GitHubApp model
  • Some permissions were missing from the InstallationPermissions model

After the change?

  • The missing properties have been added

Other information

  • I pulled the list of permissions from the OpenAPI description using the following JSONPath query: $.components.schemas.app-permissions.properties

Additional info

Pull request checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Added the appropriate label for the given change (I don't have permission to)

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

  • Yes (Please add the Type: Breaking change label)
  • No

If Yes, what's the impact:

  • Low - adding properties requires adding constructor arguments, but consumers are unlikely to be using model constructors. Would it make sense to start making the model constructors internal to prevent future breaking changes?

Pull request type

Please add the corresponding label for change this PR introduces:

  • Bugfix: Type: Bug
  • Feature/model/API additions: Type: Feature
  • Updates to docs or samples: Type: Documentation
  • Dependencies/code cleanup: Type: Maintenance

@kfcampbell kfcampbell added Type: Breaking change Used to note any change that requires a major version bump Type: Feature New feature or request labels Nov 30, 2022
kfcampbell
kfcampbell reviewed Nov 30, 2022
View reviewed changes
Octokit.Tests/Clients/MetaClientTests.cs Outdated
@@ -23,7 +23,8 @@ public async Task RequestsTheMetadataEndpoint()
new[] { "1.1.6.1/24", "1.1.6.2/24" },
new[] { "1.1.7.1", "1.1.7.2" },
new[] { "1.1.8.1/24", "1.1.8.2/24" },
new[] { "1.1.9.1", "1.1.9.2" }
new[] { "1.1.9.1", "1.1.9.2" },
null
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind explaining the addition of the null here? I'm not sure I follow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was just to satisfy the required constructor arguments. I left it as null here because installed_version isn't present on calls to github.com. However, I'm happy to change it to 3.7.0 if you'd prefer to have all the values filled in. I could also make that constructor parameter optional if that's preferable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm...I would probably prefer an explicit argument there, but I'm unable to articulate a good case for why. Matching other behavior is probably our best bet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense! I've updated it to an actual value

@kfcampbell
Copy link
Member

@nickfloyd has mentioned that in the past we typically don't treat constructor/model changes like this one as a breaking change. Is there another aspect that makes this change breaking I'm not seeing?

@MatisseHack
Copy link
Contributor Author

No, just the constructor change. I saw other PRs were approved with constructor changes so I assumed it would be okay, but still marked it as a breaking change since it seemed applicable.

kfcampbell
kfcampbell approved these changes Dec 2, 2022
View reviewed changes
Copy link
Member

@kfcampbell kfcampbell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thank you! I'll get this merged shortly and released at some point in the future.

@kfcampbell kfcampbell merged commit 0475f08 into octokit:main Dec 2, 2022
@MatisseHack
Copy link
Contributor Author

Thanks!

@MatisseHack MatisseHack deleted the meta-and-permissions branch December 2, 2022 00:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kfcampbell kfcampbell kfcampbell approved these changes

Assignees
No one assigned
Labels
Type: Breaking change Used to note any change that requires a major version bump Type: Feature New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEAT]: Add missing properties for meta and app payloads
2 participants
@MatisseHack @kfcampbell