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

Feedback on README.md instructions #2

Closed
consideRatio opened this issue Jan 25, 2022 · 6 comments
Closed

Feedback on README.md instructions #2

consideRatio opened this issue Jan 25, 2022 · 6 comments

Comments

@consideRatio
Copy link
Member

  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
Copy link
Collaborator

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
Copy link
Member Author

🎉 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
Copy link
Collaborator

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

@consideRatio
Copy link
Member Author

consideRatio commented Jan 25, 2022

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 added a commit that referenced this issue Apr 17, 2022
- Better name for what it actually does
- Prefix env var we use to pick up client id
- Rename the IPython Magic too

Ref #2
yuvipanda added a commit that referenced this issue Apr 18, 2022
Removes a fiddly client-side config! This was particularly
problematic when git was installed via conda, as it does not
read the systemwide /etc/gitconfig file
(conda-forge/git-feedstock#113)

Ref #2
@yuvipanda
Copy link
Collaborator

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 (Automatically tell git where to look for github creds #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
Copy link
Member Author

Wieee nice work!!

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

No branches or pull requests

2 participants