Add missing permissions to InstallationPermissions #1180

tarebyte · 2019-05-31T12:57:36Z

This adds the remaining GitHub App installation permissions defined in https://developer.github.com/v3/apps/permissions/#metadata-permissions that are related to GitHub App installations.

These do not include permissions such as emails or gpg_keys as these are user permissions.

I think I found the relevant test(s) to update, but if there are more I can add please let me know.

✨ Thanks ✨

This adds the remaining GitHub App installation permissions defined in https://developer.github.com/v3/apps/permissions/#metadata-permissions that are related to GitHub App installations. These do not include permissions such as `emails` or `gpg_keys` as these are user permissions.

codecov · 2019-05-31T12:59:53Z

Codecov Report

Merging #1180 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1180   +/-   ##
=======================================
  Coverage   70.25%   70.25%           
=======================================
  Files          84       84           
  Lines        5867     5867           
=======================================
  Hits         4122     4122           
  Misses        956      956           
  Partials      789      789

Impacted Files	Coverage Δ
github/apps.go	`65.51% <ø> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8354c07...a8d3df2. Read the comment docs.

gmlewis · 2019-05-31T13:46:52Z

Thank you, @tarebyte!

I'm looking around the GitHub API v3 Developer docs, and am having troubles finding these added fields. Where did you find these?
Are they really all strings?
If you have a documentation URL, could you please add it to a comment before the expanded struct?

tarebyte · 2019-05-31T14:01:15Z

I'm looking around the GitHub API v3 Developer docs, and am having troubles finding these added fields. Where did you find these?

The permissions are listed here: https://developer.github.com/v3/apps/permissions

Are they really all strings?

Yup! The string is either read/write/admin and if it's not one of those then the field isn't part of the JSON response.

An example of this would be the permissions key in this response https://developer.github.com/v3/apps/#get-an-installation

If you have a documentation URL, could you please add it to a comment before the expanded struct?

Happy to, is https://developer.github.com/v3/apps/permissions/ sufficient?

gmlewis · 2019-05-31T14:17:28Z

Hmmm... I'm still confused. Let's take an example.
Where did you find the string organization_pre_receive_hooks ?

tarebyte · 2019-05-31T14:21:13Z

Where did you find the string organization_pre_receive_hooks ?

That's an Enterprise only permission, it's located in https://developer.github.com/enterprise/2.17/v3/apps/permissions/

gmlewis · 2019-05-31T14:32:23Z

Ah! Great, thanks. Then can you please add the two URLs in the comment?
Something like:

// Hook names taken from:
//   https://developer.github.com/v3/apps/permissions/
//   https://developer.github.com/enterprise/2.17/v3/apps/permissions/

Thank you!

tarebyte · 2019-05-31T15:30:29Z

Ah! Great, thanks. Then can you please add the two URLs in the comment?

Done in a8d3df2

tarebyte · 2019-05-31T15:38:43Z

github/apps.go

+//
+// Permission names taken from:
+//   https://developer.github.com/v3/apps/permissions/
+//   https://developer.github.com/enterprise/v3/apps/permissions/


This will redirect to the latest Enterprise version.

https://developer.github.com/enterprise/v3/apps/permissions/ -> https://developer.github.com/enterprise/2.17/v3/apps/permissions/

gmlewis

LGTM.

Awaiting second LGTM before merging.

itsdalmo · 2019-06-11T12:28:45Z

github/apps.go

+	ContentReferences           *string `json:"content_references,omitempty"`
+	Deployments                 *string `json:"deployments,omitempty"`
+	Issues                      *string `json:"issues,omitempty"`
+	Metadata                    *string `json:"metadata,omitempty"`


According to the following documentation:
https://developer.github.com/apps/building-github-apps/creating-github-apps-using-url-parameters/#github-app-permissions

It seems like this should be RepositoryMetadata, and more importantly, the JSON key should be repository_metadata?

@itsdalmo that seems to be an error in the docs, the key from the response is definitely metadata.

Thanks, @itsdalmo and @tarebyte!

Would one of you please volunteer to contact [email protected] and let them know about the documentation error? They are typically very responsive, friendly, and helpful, and this would be appreciated feedback. Thank you!

Would one of you please volunteer to contact [email protected] and let them know about the documentation error?

I can file an internal issue, I work for GitHub 😃

They are typically very responsive, friendly, and helpful, and this would be appreciated feedback. Thank you!

💖

Oh, uh, Wow! I'm humbled and honored that you are helping us out here, @tarebyte!!!

You and your team have been an absolute joy to work with! I have a bunch of virtual friends at GitHub Tech Support. Thanks for all you do!

So let's see... all I need is another LGTM and we are good to merge.

Oh, uh, Wow! I'm humbled and honored that you are helping us out here, @tarebyte!!!

You and your team have been an absolute joy to work with! I have a bunch of virtual friends at GitHub Tech Support. Thanks for all you do!

It's my pleasure! Thank you for working on such a ✨ library

gmlewis · 2019-06-14T13:08:03Z

@itsdalmo - since you reviewed the code, can I please get an LGTM from you and we can merge this?

itsdalmo

Quick review, as requested by @gmlewis. I've based it on the following documentation:
https://developer.github.com/apps/building-github-apps/creating-github-apps-using-url-parameters/#github-app-permissions

According to the documentation there seems to be some missing permissions still:

blocking
emails
followers
gpg_keys
keys
packages
plan
starring
watching

With the exception of watching, all of these seem to be present in the V3 documentation as well:
https://developer.github.com/v3/apps/permissions/#github-app-permissions

Unless both docs are wrong I think we should add these missing permissions as well? 😅

tarebyte · 2019-06-17T08:06:13Z

@itsdalmo, those are all user permissions that aren’t granted to an app installation.

These are only requested and granted when a user goes through the authorization flow and the GitHub App gets a “user to server” token.

tarebyte · 2019-06-17T08:08:29Z

Ooo actually I think packages might be a installation permission.

I’ll double check.

itsdalmo

These are only requested and granted when a user goes through the authorization flow and the GitHub App gets a “user to server” token.

Thanks for clarifying @tarebyte - LGTM

gmlewis · 2019-06-19T01:09:58Z

It looks like we have two LGTMs but also some more commits coming in.

Just FYI, @tarebyte, we try not to use force pushes for PRs in this repo as it makes it more challenging for the reviewers to see what changed, but no big deal. Sometimes we have to use them to clear up the CLA checker and make it happy. 😄

So I'm just checking in with you on the status... are we good to merge, or do you think there are other changes that need making first?

tarebyte · 2019-06-19T04:29:31Z

Good to know thanks!

This is good to go from my perspective, the missing packages permission was all I needed to add.

gmlewis · 2019-06-19T13:25:11Z

Thank you, @tarebyte and @itsdalmo!
Merging.

googlebot added the cla: yes Indication that the PR author has signed a Google Contributor License Agreement. label May 31, 2019

Add links to show source of App permission names

a8d3df2

tarebyte commented May 31, 2019

View reviewed changes

gmlewis approved these changes May 31, 2019

View reviewed changes

gmlewis requested a review from gauntface May 31, 2019 15:45

itsdalmo reviewed Jun 11, 2019

View reviewed changes

itsdalmo reviewed Jun 17, 2019

View reviewed changes

itsdalmo approved these changes Jun 17, 2019

View reviewed changes

tarebyte force-pushed the additional-app-fields branch from d7d3fde to 699efa9 Compare June 19, 2019 00:03

Add packages permission

d2e7e74

tarebyte force-pushed the additional-app-fields branch from 699efa9 to d2e7e74 Compare June 19, 2019 00:09

gmlewis merged commit 7a8ff7a into google:master Jun 19, 2019

n1lesh pushed a commit to n1lesh/go-github that referenced this pull request Oct 2, 2020

Add missing permissions to InstallationPermissions (google#1180)

c200bcd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing permissions to InstallationPermissions #1180

Add missing permissions to InstallationPermissions #1180

tarebyte commented May 31, 2019

codecov bot commented May 31, 2019 •

edited

Loading

gmlewis commented May 31, 2019

tarebyte commented May 31, 2019

gmlewis commented May 31, 2019 •

edited

Loading

tarebyte commented May 31, 2019

gmlewis commented May 31, 2019 •

edited

Loading

tarebyte commented May 31, 2019

tarebyte May 31, 2019

gmlewis left a comment

itsdalmo Jun 11, 2019

tarebyte Jun 11, 2019

gmlewis Jun 11, 2019

tarebyte Jun 11, 2019

gmlewis Jun 11, 2019

tarebyte Jun 11, 2019

gmlewis commented Jun 14, 2019

itsdalmo left a comment

tarebyte commented Jun 17, 2019

tarebyte commented Jun 17, 2019

itsdalmo left a comment

gmlewis commented Jun 19, 2019 •

edited

Loading

tarebyte commented Jun 19, 2019

gmlewis commented Jun 19, 2019

Add missing permissions to InstallationPermissions #1180

Add missing permissions to InstallationPermissions #1180

Conversation

tarebyte commented May 31, 2019

codecov bot commented May 31, 2019 • edited Loading

Codecov Report

gmlewis commented May 31, 2019

tarebyte commented May 31, 2019

gmlewis commented May 31, 2019 • edited Loading

tarebyte commented May 31, 2019

gmlewis commented May 31, 2019 • edited Loading

tarebyte commented May 31, 2019

tarebyte May 31, 2019

Choose a reason for hiding this comment

gmlewis left a comment

Choose a reason for hiding this comment

itsdalmo Jun 11, 2019

Choose a reason for hiding this comment

tarebyte Jun 11, 2019

Choose a reason for hiding this comment

gmlewis Jun 11, 2019

Choose a reason for hiding this comment

tarebyte Jun 11, 2019

Choose a reason for hiding this comment

gmlewis Jun 11, 2019

Choose a reason for hiding this comment

tarebyte Jun 11, 2019

Choose a reason for hiding this comment

gmlewis commented Jun 14, 2019

itsdalmo left a comment

Choose a reason for hiding this comment

tarebyte commented Jun 17, 2019

tarebyte commented Jun 17, 2019

itsdalmo left a comment

Choose a reason for hiding this comment

gmlewis commented Jun 19, 2019 • edited Loading

tarebyte commented Jun 19, 2019

gmlewis commented Jun 19, 2019

codecov bot commented May 31, 2019 •

edited

Loading

gmlewis commented May 31, 2019 •

edited

Loading

gmlewis commented May 31, 2019 •

edited

Loading

gmlewis commented Jun 19, 2019 •

edited

Loading