Allow use of client id as an app id #4057
Conversation
nikola-jokic
added
the
gha-runner-scale-set
nikola-jokic
requested review from
mumoshu,
toast-gear,
rentziass and
a team
as code owners
There was a problem hiding this comment.
Pull Request Overview
This PR enables the use of client IDs as AppIDs by changing the type of the AppID from an integer to a string throughout the codebase while maintaining backwards compatibility. Key changes include:
- Converting AppID values in tests and production code from int to string.
- Removing integer parsing logic and updating JWT claims to use the string AppID.
- Adjusting error messages and tests to match the new string formatting.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| github/actions/multi_client_test.go | Updated test cases to use string literals for AppID |
| github/actions/multi_client.go | Changed GitHubAppAuth.AppID to string and removed integer parsing |
| github/actions/identifier_test.go | Updated identifier tests to reflect the string AppID format |
| github/actions/client.go | Modified JWT creation to use string AppID as Issuer |
| controllers/actions.github.com/resourcebuilder.go | Updated secret retrieval to convert AppID to string and use maps.Copy |
| cmd/ghalistener/config/config_test.go | Adjusted error message formatting in tests |
| cmd/ghalistener/config/config.go | Updated error messages and validation for string-based AppID |
| @@ -18,7 +18,7 @@ import ( | |||
|
|
|||
| type Config struct { | |||
| ConfigureUrl string `json:"configure_url"` | |||
| AppID int64 `json:"app_id"` | |||
There was a problem hiding this comment.
We should probably clarify in the values.yaml file that this field can be either the app id or client id
| @@ -23,7 +23,7 @@ type multiClient struct { | |||
| } | |||
|
|
|||
| type GitHubAppAuth struct { | |||
| AppID int64 | |||
| AppID string | |||
There was a problem hiding this comment.
nit: Do we want to clarify that this could be both AppID or ClientID?
|
|
||
| if !hasToken && !hasPrivateKeyConfig { | ||
| return fmt.Errorf("GitHub auth credential is missing, token length: '%d', appId: '%d', installationId: '%d', private key length: '%d", len(c.Token), c.AppID, c.AppInstallationID, len(c.AppPrivateKey)) | ||
| return fmt.Errorf(`GitHub auth credential is missing, token length: "%d", appId: %q, installationId: "%d", private key length: "%d"`, len(c.Token), c.AppID, c.AppInstallationID, len(c.AppPrivateKey)) |
There was a problem hiding this comment.
Do we want to clarify here that we expect an AppID or a ClientID?
|
I pushed a few comments explaining that the app_id can be a client id |
This change allows client ID not to be an integer only, but also to be a client ID (see https://github.blog/changelog/2024-05-01-github-apps-can-now-use-the-client-id-to-fetch-installation-tokens/).
This change is backwards-compatible.
Fixes #3667