-
Couldn't load subscription status.
- Fork 4
Fixed github auth stuff so it actually works #21
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
Conversation
|
This needs the same OAuth app creds as before and then GITHUB_APP_ID is the id of a github app and GITHUB_APP_PRIVATE_KEY is a private key of the app. The app needs perms read write org members and should be installed on the csh org |
|
Noticing one of the linting things messed up a lot of the formatting I could revert that I'm not entirely sure how that happened since the previous commits were being linted and passing before? |
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.
Looks wonderful, just a nitpick
|
I'm confused why yapf took such a substantial change to everything, given that it hadn't complained about mom/dev before. Part of the reason I chose it was that it's pep8 style, and wouldn't make substantial revisions. If you give me a ping in the morning, I'd be happy to give a review |
|
@mxmeinhold yeah I am not quite sure apparently it's maybe not deterministic? |
|
Ah I see, in 1f019f8, things were moved to paratheses, at which point the formatter won't maintain the newlines -_SLACK_AUTH_URI = 'https://slack.com/oauth/authorize' \
- + '?scope=identity.basic' \
- + '&client_id=%s' \
- + '&state=%s' \
- + '&redirect_uri=https://eac.csh.rit.edu/slack/return'
+_SLACK_AUTH_URI = (
+ "https://slack.com/oauth/authorize"
+ + "?scope=identity.basic"
+ + "&client_id=%s"
+ + "&state=%s"
+ + "&redirect_uri=https://eac.csh.rit.edu/slack/return"
+)``` |
|
Alright I think I got it figured out, sent you an email |
|
okay there we go I fixed it hopefully and the diff is better I think I just ran the wrong linting commands when I was attempting to get it to pass because I didn't see the stuff in the readme |
|
also ngl I didn't see your email until now but I think looking at the patches we did the same thing 🤷 |
|
sorry I can't figure out how to request a review so @mxmeinhold |
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.
take a glance at if you need to bump csh ldap, otherwise looking good
requirements.txt
Outdated
| @@ -1,4 +1,4 @@ | |||
| csh-ldap==2.4.0 | |||
| csh-ldap @ git+https://github.com/ComputerScienceHouse/[email protected].0 | |||
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.
I'd bump to 2.5.1 for this fix ComputerScienceHouse/csh_ldap#39
|
@BigSpaceships I'm fine with this getting merged, did you get someone to add the token config in okd? |
|
@costowell Can you add a not dev private key and client id to okd? The same perms the old one should work fine lmk if you need help with what is what and what we did last time |
|
Looks epic, I have created a non-dev Github app.
|
crying in whatever the auth system is for github apps
also I'll fix the merge conflicts but I can't figure out how to do it without making a pr