Skip to content

Commit

Permalink
Do not nest transports for Github installation client (#1564)
Browse files Browse the repository at this point in the history
#1454 modified one of the Github enumeration code paths in a way that broke an integration test by causing one client's transport to be used for the construction of a different client, causing authentication failures. This saves the original transport for use, fixing the test.
  • Loading branch information
rosecodym committed Jul 31, 2023
1 parent e0faac8 commit ad57de5
Showing 1 changed file with 17 additions and 14 deletions.
31 changes: 17 additions & 14 deletions pkg/sources/github/github.go
Original file line number Diff line number Diff line change
Expand Up @@ -556,38 +556,41 @@ func (s *Source) enumerateWithApp(ctx context.Context, apiEndpoint string, app *
return nil, errors.New(err)
}

// This client is used for most APIs.
itr, err := ghinstallation.New(
// This client is required to create installation tokens for cloning.
// Otherwise, the required JWT is not in the request for the token :/
// This client uses the source's original HTTP transport, so make sure
// to build it before modifying that transport (such as is done during
// the creation of the other API client below).
appItr, err := ghinstallation.NewAppsTransport(
s.httpClient.Transport,
appID,
installationID,
[]byte(app.PrivateKey))
if err != nil {
return nil, errors.New(err)
}
itr.BaseURL = apiEndpoint
appItr.BaseURL = apiEndpoint

s.httpClient.Transport = itr
s.apiClient, err = github.NewEnterpriseClient(apiEndpoint, apiEndpoint, s.httpClient)
// Does this need to be separate from |s.httpClient|?
instHttpClient := common.RetryableHttpClientTimeout(60)
instHttpClient.Transport = appItr
installationClient, err = github.NewEnterpriseClient(apiEndpoint, apiEndpoint, instHttpClient)
if err != nil {
return nil, errors.New(err)
}

// This client is required to create installation tokens for cloning.
// Otherwise, the required JWT is not in the request for the token :/
appItr, err := ghinstallation.NewAppsTransport(
// This client is used for most APIs.
itr, err := ghinstallation.New(
s.httpClient.Transport,
appID,
installationID,
[]byte(app.PrivateKey))
if err != nil {
return nil, errors.New(err)
}
appItr.BaseURL = apiEndpoint
itr.BaseURL = apiEndpoint

// Does this need to be separate from |s.httpClient|?
instHttpClient := common.RetryableHttpClientTimeout(60)
instHttpClient.Transport = appItr
installationClient, err = github.NewEnterpriseClient(apiEndpoint, apiEndpoint, instHttpClient)
s.httpClient.Transport = itr
s.apiClient, err = github.NewEnterpriseClient(apiEndpoint, apiEndpoint, s.httpClient)
if err != nil {
return nil, errors.New(err)
}
Expand Down

0 comments on commit ad57de5

Please sign in to comment.