search

jupyterhub / gh-scoped-creds

Provide fine-grained push access to GitHub from a JupyterHub
BSD 3-Clause "New" or "Revised" License
25 stars 7 forks source link

Feedback on README.md instructions #2

Closed consideRatio closed 2 years ago

consideRatio commented 2 years ago

  1. Step 6 in the GitHub App configuration, part of the README.md includes:

    Save the Public link as well, as users will need to use this to grant access to particular repositories.

    What does this refer to?

  2. Step 1 in the Client configuration, part of the README.md refers to use of git-credentials-store. Since that is a technical reference, I'd expect a link or similar to be able to understand if that was a Python package we depend on, an apt-get package, or something part of what you get when installing git in any way etc.

    Is it correct that git-credentials-store refers to something shipping with git, namely this.

  3. Use of GITHUB_APP_CLIENT_ID, maybe GITHUB_APP_USER_AUTH_CLIENT_ID or GITHUB_APP_USER_AUTH__CLIENT_ID instead? I've seen a lot of confusion stemming from not knowing what environment variables influence what software. By prefixing the environment variable with the related software's full name, there is a lot less confusion about it. To me, that far outweighs the downside of having a longer environment variable name.

    What do you think about renaming this env var?

yuvipanda commented 2 years ago

Great feedback, @consideRatio!

(1) refers to this:

image

This is the link used by users to 'install' the app to grant it access to particular repos.

For (2), adding the link seems a great idea.

For (3), +1 - let's change it now, it's early enough that we have control of all the major deployments :D I like GITHUB_APP_USER_AUTH_CLIENT_ID

consideRatio commented 2 years ago

:tada: thanks for the clarifications and project!

About renaming the environment variable. Perhaps we can rename also the suggested filename as well? From --file=/tmp/github-app-git-credentials to --file=/tmp/github-app-user-auth-git-credentials.

yuvipanda commented 2 years ago

yep, both sound like great ideas. Do you think you can make a quick PR, @consideRatio?

consideRatio commented 2 years ago

Speaking of renaming. I also consider a project name like github-tmp-scoped-user-auth, github-tmp-user-auth, github-tmp-scoped-user-credentials, github-tmp-scoped-cred-helper, etc.?

The fact that we rely on a GitHub application feels less relevant to convey than that what is enabled for the by using this - temporary, scoped, github credentials.

Just brainstorming a bit!

@yuvipanda I'll make sure to get this up and running and such first. I've already branched out to this issue etc before having it up and running. I'm happy to submit these kinds of smaller PRs though!

yuvipanda commented 2 years ago

I've done the following:

  1. Renamed the project to gh-scoped-creds!
  2. Made the environment variable be GH_SCOPED_CREDS_CLIENT_ID so it's not super generic
  3. The appropriate gitconfig entry is now automatically set (https://github.com/yuvipanda/gh-scoped-creds/pull/14) so admins no longer need to configure it.
  4. I've linked to git-credentials-store from the README.

I think the public app 'install' link can be made more discoverable, but you've already opened #4 - so I'm going to close this one. THANK YOU SO MUCH for making the project better! <3

consideRatio commented 2 years ago

Wieee nice work!!