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.

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

Add support for creating scoped installation tokens #1182

Closed
wants to merge 2 commits into from

Conversation

tarebyte
Copy link
Contributor

@tarebyte tarebyte commented Jun 5, 2019

See https://developer.github.com/v3/apps/#create-a-new-installation-token

This creates a new function CreateScopedInstallationToken, which allows the caller to limit an installation's access to a subset of repositories and permissions.

These parameters are optional which is why I opted to created a new function for this.

✨ Feedback appreciated ✨

See https://developer.github.com/v3/apps/#create-a-new-installation-token

This creates a new function `CreateScopedInstallationToken`, which allows
the caller to limit an installation's access to a subset of
repositories and permissions.
@googlebot googlebot added the cla: yes Indication that the PR author has signed a Google Contributor License Agreement. label Jun 5, 2019
@codecov
Copy link

codecov bot commented Jun 5, 2019

Codecov Report

Merging #1182 into master will decrease coverage by 0.01%.
The diff coverage is 63.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1182      +/-   ##
==========================================
- Coverage   70.25%   70.24%   -0.02%     
==========================================
  Files          84       84              
  Lines        5867     5878      +11     
==========================================
+ Hits         4122     4129       +7     
- Misses        956      958       +2     
- Partials      789      791       +2
Impacted Files Coverage Δ
github/apps.go 65.3% <63.63%> (-0.22%) ⬇️

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...e1d27a0. Read the comment docs.

Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thank you, @tarebyte!

We try to keep the surface area of the API as small as possible (to make maintenance easier, to make it easier for the user to find a function they need, and to keep the auto-generated godocs as small as possible), and prefer to break the API and bump the major version number.

I would like to request that you add the new parameter to the existing endpoint above it, but before you make this non-trivial change, let's see if @gauntface agrees with me or if he prefers to keep it separate like you've made it.

Otherwise, LGTM.

@gmlewis gmlewis requested a review from gauntface June 14, 2019 13:09
@gmlewis
Copy link
Collaborator

gmlewis commented Jun 19, 2019

friendly ping @gauntface

@gauntface
Copy link
Contributor

I would agree with @gmlewis that moving to a single struct would make this more consistent with the rest of the API. Otherwise, this change is LGTM from me

@shanempope shanempope mentioned this pull request Jul 24, 2019
@gmlewis gmlewis closed this in 61d9e5b Aug 24, 2019
n1lesh pushed a commit to n1lesh/go-github that referenced this pull request Oct 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Indication that the PR author has signed a Google Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants