Skip to content

fix(auth): app_id can be an str#146

Merged
bhearsum merged 2 commits intomozilla-releng:mainfrom
shtrom:app_id-str
Mar 31, 2026
Merged

fix(auth): app_id can be an str#146
bhearsum merged 2 commits intomozilla-releng:mainfrom
shtrom:app_id-str

Conversation

@shtrom
Copy link
Copy Markdown
Contributor

@shtrom shtrom commented Mar 30, 2026

@shtrom shtrom requested a review from a team as a code owner March 30, 2026 04:13
Copy link
Copy Markdown
Contributor

@bhearsum bhearsum left a comment

Choose a reason for hiding this comment

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

I think the reason this is typed as an int, is because all valid github app ids are ints AFAIK, even though it goes over the wire as a string in the end, so only accepting ints is a form of validation that callers are doing the right thing.

With that said, I imagine that this is often is read by callers from places that are strings...and it can be annoying to cast just to have the value cast back to a string internally.

@ahal - maybe you can tiebreak this?

@shtrom
Copy link
Copy Markdown
Contributor Author

shtrom commented Mar 31, 2026

My app_id is very much not a int (:

Correction: my App ID is indeed an int, but my Client ID isn't. The GitHub endpoint for token generation accepts both, but recommends the use of the (str) Client ID https://docs.github.com/en/apps/creating-github-apps/authenticating-with-a-github-app/generating-a-json-web-token-jwt-for-a-github-app#:~:text=Use%20of%20the%20client%20ID%20is%20recommended%2E

@shtrom
Copy link
Copy Markdown
Contributor Author

shtrom commented Mar 31, 2026

Maybe we should rename that parameter to client_or_app_id, but that may not be backward compatible.

EDIT: I updated the docstring for app_id to reflect the polymorphism and GitHub's recommendation.

@bhearsum bhearsum merged commit b625783 into mozilla-releng:main Mar 31, 2026
6 checks passed
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

Successfully merging this pull request may close these issues.

app_id can be an str

2 participants