search

octokit / auth-app.js

GitHub App authentication for JavaScript
MIT License
147 stars 50 forks source link

feat: `appId` argument can be set to Client ID string #606

Closed oscard0m closed 4 months ago

oscard0m commented 4 months ago

Resolves #603

Before the change?

appId only allowed number as type.

After the change?

appId allows number or string as type.

Pull request checklist

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

github-actions[bot] commented 4 months ago

👋 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! 🚀

wolfy1339 commented 4 months ago

Would it be a good idea to bump universal-github-app-jwt to 2.2

gr2m commented 4 months ago

Would it be a good idea to bump universal-github-app-jwt to 2.2

yes, please! Bump it to ^2.2.0 to make sure that it supports the string ID: https://github.com/gr2m/universal-github-app-jwt/releases/tag/v2.2.0

oscard0m commented 4 months ago

Would it be a good idea to bump universal-github-app-jwt to 2.2

yes, please! Bump it to ^2.2.0 to make sure that it supports the string ID: https://github.com/gr2m/universal-github-app-jwt/releases/tag/v2.2.0

Isn't v2.2.0 of universal-github-app-jwt already in place? https://github.com/octokit/auth-app.js/blob/81ac6ecd51985906888c31b4f43c83402ffc03ef/package-lock.json#L6201-L6202 introduced by Renovate in https://github.com/octokit/auth-app.js/pull/604

wolfy1339 commented 4 months ago

Would it be a good idea to bump universal-github-app-jwt to 2.2

yes, please! Bump it to ^2.2.0 to make sure that it supports the string ID: https://github.com/gr2m/universal-github-app-jwt/releases/tag/v2.2.0

Isn't v2.2.0 of universal-github-app-jwt already in place?

https://github.com/octokit/auth-app.js/blob/81ac6ecd51985906888c31b4f43c83402ffc03ef/package-lock.json#L6201-L6202 introduced by Renovate in #604

That is only the package-lock.json which isn't published. Consumers rely on the dependencies listed in package.json

gr2m commented 4 months ago

@oscard0m if it's done from your side, can you let me merge it please? I want to test out the latest semantic-release beta version in a real-life environment

oscard0m commented 4 months ago

@oscard0m if it's done from your side, can you let me merge it please? I want to test out the latest semantic-release beta version in a real-life environment

go for it @gr2m

github-actions[bot] commented 4 months ago

:tada: This PR is included in version 7.1.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

joebowbeer commented 1 month ago

@oscard0m should I create an issue to update the README?

By my reading, the appId is required and clientId is optional, which confused me.

I didn't see any mention that clientId could be assigned to appId.

gr2m commented 1 month ago

@oscard0m should I create an issue to update the README?

that would be great, thank you!