Skip to content
This repository was archived by the owner on Aug 30, 2023. It is now read-only.

sentry should acquire less permission #14

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

icedfish
Copy link

@icedfish icedfish commented Jul 28, 2016

It's really toooooo much to get member's private repo access.


This change is Reviewable

@codecov-io
Copy link

Current coverage is 49.00% (diff: 100%)

Merging #14 into master will not change coverage

@@             master        #14   diff @@
==========================================
  Files             9          9          
  Lines           200        200          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits             98         98          
  Misses          102        102          
  Partials          0          0          

Powered by Codecov. Last update 397a658...fa7c4b8

@dcramer
Copy link
Member

dcramer commented Aug 4, 2016

@icedfish repo is requested right now because sentry-github needs it. We could probably be smarter about this, but I feel like the only way we could really do that is by having sentry-auth-github and sentry-github being the same project.

@streaky
Copy link

streaky commented Nov 8, 2016

Is not the sane thing to do here to check for GITHUB_EXTENDED_PERMISSIONS as used elsewhere and if it exists and is empty then don't include the repo scope and obviously if it has repo then ask for repo perms (and I guess potentially others)?

GITHUB_EXTENDED_PERMISSIONS = [] # don't ask for repo perms
GITHUB_EXTENDED_PERMISSIONS = ['repo'] # do ask for repo perms

Otherwise behave as-is

Just a thought..

@Shelvak
Copy link

Shelvak commented Dec 12, 2017

You could really improve this changing that line with:

SCOPE = ','.join(filter(None, ['user:email', getattr(settings, 'GITHUB_REQUIRE_PERMS', None)]))

In some cases Gh is used just for login credentials.

Cheers

@AudriusButkevicius
Copy link

Given these two plugins are now separate, and this scope has no effect on github integration, I don't see a reason why this should not be merged. I've been running this for a while and it works perfectly fine.

@palmerabollo
Copy link

Can this contribution be reviewed? I think it's an interesting feature and merging it would avoid some forks. Thank you.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants