-
Notifications
You must be signed in to change notification settings - Fork 135
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
Expose User/Org information under site.github.owner #151
Conversation
Update: Fixed the test case, so now CI tests pass. I also managed to test this on a project page (by setting env variable I investigated filtering fields such as email (using Thanks! |
I think we'd feel safer with an allow list. This would also allow us to upgrade to GraphQL in the future. Here's what I think we should allow: {
"login": "octocat",
"id": 1,
"node_id": "MDQ6VXNlcjE=",
"avatar_url": "https://github.com/images/error/octocat_happy.gif",
"html_url": "https://github.com/octocat",
"type": "User",
"site_admin": false,
"name": "monalisa octocat",
"company": "GitHub",
"blog": "https://github.com/blog",
"location": "San Francisco",
"bio": "There once was...",
"public_repos": 2,
"public_gists": 1,
"followers": 20,
"following": 0,
"created_at": "2008-01-14T04:33:35Z",
"updated_at": "2008-01-14T04:33:35Z"
} For organizations, let's allow only the following fields: {
"login": "github",
"id": 1,
"node_id": "MDEyOk9yZ2FuaXphdGlvbjE=",
"url": "https://api.github.com/orgs/github",
"avatar_url": "https://github.com/images/error/octocat_happy.gif",
"description": "A great organization",
"name": "github",
"company": "GitHub",
"blog": "https://github.com/blog",
"location": "San Francisco",
"email": "[email protected]",
"is_verified": true,
"has_organization_projects": true,
"has_repository_projects": true,
"public_repos": 2,
"public_gists": 1,
"followers": 20,
"following": 0,
"html_url": "https://github.com/octocat",
"created_at": "2008-01-14T04:33:35Z",
"type": "Organization",
"collaborators": 8,
} What do you think? |
Sure @parkr that list looks pretty reasonable. I can work on an update and push to this PR. Thanks! |
a23475c
to
d2e5a35
Compare
Done. Please take a look. The AppVeyor failures look unrelated... Can you please relaunch those two failing jobs? |
@filbranden Done! |
:updated_at, | ||
]) | ||
|
||
def owner_obj |
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.
Instead of owner_obj, how about calling this owner_metadata
? A comment above the method would also help tremendously! I'm also tempted to move this user lookup & filter into a separate Drop class...
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.
Done.
(When first writing the code, I was thinking of a follow up that would rename owner
to owner_name
to make those match... It's a much more intrusive patch, but maybe it would be nice to have the same name on either side?)
In any case, renamed as requested. Please take another look.
AppVeyor is 1/4 unhappy... Please restart that build if possible? Thanks! |
Thank you so much! @jekyllbot: merge +minor |
Awesome, thanks a lot! |
Fixes #149.
Tested locally for a User object, captured everything I was interested in (full name, email, company, bio, ...)
/cc @parkr @benbalter
Thanks!
Filipe