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] add getInstallationUrl method #542

Merged
merged 16 commits into from
Jun 6, 2024
Merged

[FEAT] add getInstallationUrl method #542

merged 16 commits into from
Jun 6, 2024

Conversation

rpmccarter
Copy link
Contributor

@rpmccarter rpmccarter commented May 13, 2024

Resolves #541


Before the change?

  • determining the installation url for the github app requires searching the docs and finding both the required installation url and the GET /app endpoint for getting the metadata for the current app

After the change?

  • the app.getInstallationUrl({ state, target_id }) method crafts the installation url for the user

Pull request checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

  • Yes
  • No

Don't love how getInstallationUrl looks so similar to getInstallationOctokit but "installation" means something different. octokit/oauth-methods.js uses getWebFlowAuthorizationUrl but getWebFlowInstallationUrl seemed a little unnecessarily long

The other caveat is that the base url gets cached, so it won't update if someone updates the name of their github app while the program is running - not sure how common github app renames are

Copy link

👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labled with Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀

@rpmccarter rpmccarter marked this pull request as ready for review May 13, 2024 21:01
README.md Outdated Show resolved Hide resolved
@rpmccarter rpmccarter requested a review from gr2m May 14, 2024 02:14
@gr2m
Copy link
Contributor

gr2m commented May 14, 2024

Question: how is the state argument used? Is it passed to the post installation setup URL? I've never used it ... is that documented somewhere?

@rpmccarter
Copy link
Contributor Author

rpmccarter commented May 14, 2024

@gr2m yep, the state argument is preserved and passed to subsequent redirect urls (link to the docs). In my case, we request user oauth authorization during our installation step, so the state gets passed in the oauth redirect url

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 is a great PR, thank you for the additional clarification in the README. Just one comment

src/types.ts Outdated Show resolved Hide resolved
@rpmccarter rpmccarter requested a review from gr2m May 19, 2024 00:34
@rpmccarter
Copy link
Contributor Author

@gr2m is there anything left to do on my side to get this merged? I'm unable to merge myself as I don't have write access

@wolfy1339 wolfy1339 enabled auto-merge (squash) June 6, 2024 20:52
@wolfy1339 wolfy1339 merged commit 691ccfb into octokit:main Jun 6, 2024
5 checks passed
Copy link

github-actions bot commented Jun 6, 2024

🎉 This PR is included in version 15.1.0 🎉

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
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[FEAT]: method for generating installation url
3 participants