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

feat(repositoryNames) Able to filter out what repos the Installation Token has access to via repo name #279

Merged
merged 9 commits into from
Apr 23, 2021

Conversation

NickLiffen
Copy link
Contributor

  • Repo Name is now an option.

@ghost ghost added this to Inbox in JS Apr 22, 2021
@NickLiffen
Copy link
Contributor Author

@wolfy1339 & @gr2m no rush but when you get a chance I would love your thoughts on this pull request 👍 I welcome you to test/propose any changes 👍 I think it looks good and I have tried to make it match the style of this repository. However, welcome your feedback 💯

@NickLiffen
Copy link
Contributor Author

Closes #277

@gr2m gr2m added the Type: Feature New feature or request label Apr 22, 2021
@ghost ghost moved this from Inbox to Features in JS Apr 22, 2021
Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

This looks great so far, thank you Nick!

3 things I'd suggest we add:

  1. I think I'd add a (recommended) to the repositoryId option.

    One reason is that repository names can change, repository IDs cannot. If both are available, the user should use the ids.

    Another reason is that the repository names or IDs (depending on what was passed) is used for caching IDs. Repository names can get long, repository ID lengths at least are predictable. This has an impact if e.g. Redis is used as a cache backend, but for widely used apps it could also impact memory usage.

  2. Could you add a test where you set both, repositoryIds and repositoryNames, to see what happens, and to keep that behavior consistent?

  3. Could you add to the descriptions in the README and typescript that either repositoryIds or repositoryNames should be set, not both? You can add TypeScript descriptions to options by adding a /** comment herer */ above the option, I think

@wolfy1339 wolfy1339 linked an issue Apr 22, 2021 that may be closed by this pull request
@NickLiffen
Copy link
Contributor Author

This looks great so far, thank you Nick!

3 things I'd suggest we add:

  1. I think I'd add a (recommended) to the repositoryId option.
    One reason is that repository names can change, repository IDs cannot. If both are available, the user should use the ids.
    Another reason is that the repository names or IDs (depending on what was passed) is used for caching IDs. Repository names can get long, repository ID lengths at least are predictable. This has an impact if e.g. Redis is used as a cache backend, but for widely used apps it could also impact memory usage.
  2. Could you add a test where you set both, repositoryIds and repositoryNames, to see what happens, and to keep that behavior consistent?
  3. Could you add to the descriptions in the README and typescript that either repositoryIds or repositoryNames should be set, not both? You can add TypeScript descriptions to options by adding a /** comment herer */ above the option, I think

@gr2m

  1. I agree that repositoryId is a preferred input, I will make sure to add that 👍

  2. I will add a unit test 100% for that, good shout 👍

  3. Yes, I will add that, I think that's a good idea 👍 Do you think this module should opinionate any behaviour? Like only send the repositoryIds if both are present. Or, do you think we should leave that up to the API to make the decision. If that makes sense? I am happy to be flexible 👍

I will make the changes first thing tomorrow (UK time) and send a PR your way after.

@gr2m
Copy link
Contributor

gr2m commented Apr 22, 2021

  1. Do you think this module should opinionate any behaviour? Like only send the repositoryIds if both are present. Or, do you think we should leave that up to the API to make the decision

When in doubt, I like to stay as close as possible to GitHub's APIs. Let's just pass it through and let GitHub's API respond with a helpful error.

@NickLiffen NickLiffen requested a review from gr2m April 23, 2021 07:10
@NickLiffen
Copy link
Contributor Author

Sounds good @gr2m 👍 I have made the changes 👍 Appreciate the feedback 💯 Let me know if you have any more thoughts.

Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

Looks great, thanks a lot Nick!

@gr2m gr2m merged commit e2a5503 into octokit:master Apr 23, 2021
JS automation moved this from Features to Done Apr 23, 2021
@github-actions
Copy link
Contributor

🎉 This PR is included in version 3.4.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New feature or request
Projects
No open projects
JS
  
Done
Development

Successfully merging this pull request may close these issues.

Repository ID's to be passed into: Installation authentication
2 participants