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

bug: fix private repo listing for a user #16

Merged
merged 1 commit into from
Jul 8, 2022

Conversation

dhellmann
Copy link
Contributor

@dhellmann dhellmann commented Jul 3, 2022

Private repositories are not included in the /users/$name/repos API
results. Use /user/repos?affiliation=owner instead to list only owned
repositories for the current authenticated user.

See https://docs.github.com/en/rest/repos/repos#list-repositories-for-a-user which says "Lists public repositories for the specified user" and https://docs.github.com/en/rest/repos/repos#list-repositories-for-the-authenticated-user for info on the affiliation argument.

@dhellmann dhellmann force-pushed the fix-private-repo-list branch 3 times, most recently from 4b8b0f3 to 4c321dc Compare July 3, 2022 20:05
Private repositories are not included in the /users/$name/repos API
results. Use /user/repos?affiliation=owner instead to list only owned
repositories for the current authenticated user, including those
marked private.
@dhellmann dhellmann force-pushed the fix-private-repo-list branch from 4c321dc to ecbdcd6 Compare July 3, 2022 20:08
@dhellmann
Copy link
Contributor Author

I'm not sure what's wrong with the pypy jobs. Those failures don't look related to the change, do they normally pass?

@mgedmin
Copy link
Owner

mgedmin commented Jul 8, 2022

Sorry for the delayed response, I'm on vacation and completely forgot to check my mail this week. (Not that I'm too sorry, that's the whole point of a vacation.)

I'm not sure what's wrong with the pypy jobs. Those failures don't look related to the change, do they normally pass?

Those are broken on git master as well. AFAICT what happened was that a newer GitHub Actions base image with a newer PyPy version triggers a bug in cattrs that causes those failures. cattrs has an (unreleased) fix for it, details at python-attrs/cattrs#253

Copy link
Owner

@mgedmin mgedmin left a comment

Choose a reason for hiding this comment

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

LGTM!

@mgedmin mgedmin merged commit de47740 into mgedmin:master Jul 8, 2022
# user instead. This only works if the current token
# is associated with that user.
list_url = ('https://api.github.com/user/repos'
'?affiliation=owner&sort=full_name')
Copy link
Owner

Choose a reason for hiding this comment

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

Uh, just to double-check, this branch is taken only when you provide a GitHub token?

Because by default include_private is True, but github_token is not set.

What does the GitHub API return if you try to GET /user/repos without any token? Which user does it pick?

I think I may have been too hasty in merging this...

Copy link
Owner

Choose a reason for hiding this comment

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

What does the GitHub API return if you try to GET /user/repos without any token? Which user does it pick?

Yeah, you get a 401 Unauthorized.

I'll add a fix.

Copy link
Owner

Choose a reason for hiding this comment

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

Fixed in 5048af6. I'd appreciate some testing that I haven't actually broken your use case with my fix ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, yeah, this effectively requires the GitHub token when include_private is true. That's a breaking change in behavior, so I should have highlighted it.

I'd be happy to collaborate on improving that (docs, validation, etc.).

@mgedmin
Copy link
Owner

mgedmin commented Jul 13, 2022

I've been thinking, and I'm uneasy about how the username passed to --user is ignored when --github-token is provided and --include-private is true (which it is by default). If the named user doesn't match the token, we should abort with an error message telling the user to either use the right --user, or switch to --exclude-private.

Is there a way to ask the GitHub API about the user name of the user whose token we're using to authenticate?

@dhellmann
Copy link
Contributor Author

It looks like the /user endpoint would to that. https://docs.github.com/en/developers/apps/building-github-apps/identifying-and-authorizing-users-for-github-apps

I can work on adding validation. Should that happen on unit as well as when actually querying?

@dhellmann
Copy link
Contributor Author

It looks like the /user endpoint would to that. https://docs.github.com/en/developers/apps/building-github-apps/identifying-and-authorizing-users-for-github-apps

I can work on adding validation. Should that happen on unit as well as when actually querying?

I decided to only do the validation at runtime when the query would be invoked, because the init code path doesn't already run any queries and some users will just edit the file by hand without using the init option. See #19

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

Successfully merging this pull request may close these issues.

2 participants