Skip to content

Commit 8fb3fee

Browse files
mathur07mathur07
committed
chore: Add eventEmitter parameter to NewInstallation
- Add eventEmitter parameter to NewInstallation for better error visibility - Emit events when GitHub App token generation fails - Add comprehensive test coverage for various scenarios - Handle token generation failures gracefully while returning installation ID - Keep simple, direct URL parsing approach for GitHub-specific validation
1 parent 8cdbaff commit 8fb3fee

File tree

2 files changed

+78
-22
lines changed

2 files changed

+78
-22
lines changed

‎pkg/provider/github/app/token.go‎

Lines changed: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -10,29 +10,33 @@ import (
1010

1111
"github.com/golang-jwt/jwt/v4"
1212
"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1"
13+
"github.com/openshift-pipelines/pipelines-as-code/pkg/events"
1314
"github.com/openshift-pipelines/pipelines-as-code/pkg/params"
1415
"github.com/openshift-pipelines/pipelines-as-code/pkg/provider/github"
16+
"go.uber.org/zap"
1517
"knative.dev/pkg/logging"
1618
)
1719

1820
type Install struct {
19-
request *http.Request
20-
run *params.Run
21-
repo *v1alpha1.Repository
22-
ghClient *github.Provider
23-
namespace string
21+
request *http.Request
22+
run *params.Run
23+
repo *v1alpha1.Repository
24+
ghClient *github.Provider
25+
namespace string
26+
eventEmitter *events.EventEmitter
2427
}
2528

26-
func NewInstallation(req *http.Request, run *params.Run, repo *v1alpha1.Repository, gh *github.Provider, namespace string) *Install {
29+
func NewInstallation(req *http.Request, run *params.Run, repo *v1alpha1.Repository, gh *github.Provider, namespace string, eventEmitter *events.EventEmitter) *Install {
2730
if req == nil {
2831
req = &http.Request{}
2932
}
3033
return &Install{
31-
request: req,
32-
run: run,
33-
repo: repo,
34-
ghClient: gh,
35-
namespace: namespace,
34+
request: req,
35+
run: run,
36+
repo: repo,
37+
ghClient: gh,
38+
namespace: namespace,
39+
eventEmitter: eventEmitter,
3640
}
3741
}
3842

@@ -95,7 +99,14 @@ func (ip *Install) GetAndUpdateInstallationID(ctx context.Context) (string, stri
9599
installationID := *installation.ID
96100
token, err := ip.ghClient.GetAppToken(ctx, ip.run.Clients.Kube, enterpriseHost, installationID, ip.namespace)
97101
if err != nil {
98-
logger.Warnf("Could not get a token for installation ID %d: %v", installationID, err)
102+
msg := fmt.Sprintf("Could not get a token for installation ID %d: %v", installationID, err)
103+
logger.Warn(msg)
104+
105+
// Emit an event for better visibility of GitHub App token issues
106+
if ip.eventEmitter != nil {
107+
ip.eventEmitter.EmitMessage(ip.repo, zap.WarnLevel, "GitHubAppTokenGenerationFailed", msg)
108+
}
109+
99110
// Return with the installation ID even if token generation fails,
100111
// as some operations might only need the ID.
101112
return enterpriseHost, "", installationID, nil

‎pkg/provider/github/app/token_test.go‎

Lines changed: 55 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,7 @@ func Test_GenerateJWT(t *testing.T) {
144144
},
145145
}
146146

147-
ip := NewInstallation(httptest.NewRequest(http.MethodGet, "http://localhost", strings.NewReader("")), run, &v1alpha1.Repository{}, &github.Provider{}, tt.namespace.GetName())
147+
ip := NewInstallation(httptest.NewRequest(http.MethodGet, "http://localhost", strings.NewReader("")), run, &v1alpha1.Repository{}, &github.Provider{}, tt.namespace.GetName(), nil)
148148
token, err := ip.GenerateJWT(ctx)
149149
if tt.wantErr {
150150
assert.Assert(t, err != nil)
@@ -206,7 +206,7 @@ func Test_GetAndUpdateInstallationID(t *testing.T) {
206206
ctx = info.StoreCurrentControllerName(ctx, "default")
207207
ctx = info.StoreNS(ctx, testNamespace.GetName())
208208

209-
ip := NewInstallation(httptest.NewRequest(http.MethodGet, "http://localhost", strings.NewReader("")), run, &v1alpha1.Repository{}, &github.Provider{}, testNamespace.GetName())
209+
ip := NewInstallation(httptest.NewRequest(http.MethodGet, "http://localhost", strings.NewReader("")), run, &v1alpha1.Repository{}, &github.Provider{}, testNamespace.GetName(), nil)
210210
jwtToken, err := ip.GenerateJWT(ctx)
211211
assert.NilError(t, err)
212212
req := httptest.NewRequest(http.MethodGet, "http://localhost", strings.NewReader(""))
@@ -256,7 +256,7 @@ func Test_GetAndUpdateInstallationID(t *testing.T) {
256256
`{"total_count": 2,"repositories": [{"id":1,"html_url": "https://matched/%s/incoming"},{"id":2,"html_url": "https://anotherrepo/that/would/failit"}]}`,
257257
orgName)
258258
})
259-
ip = NewInstallation(req, run, repo, gprovider, testNamespace.GetName())
259+
ip = NewInstallation(req, run, repo, gprovider, testNamespace.GetName(), nil)
260260
_, token, installationID, err := ip.GetAndUpdateInstallationID(ctx)
261261
assert.NilError(t, err)
262262
assert.Equal(t, installationID, int64(wantID))
@@ -294,6 +294,24 @@ func TestGetAndUpdateInstallationID_Fallbacks(t *testing.T) {
294294
skip bool
295295
expectedErrorString string
296296
}{
297+
{
298+
name: "repo installation succeeds directly",
299+
repoURL: fmt.Sprintf("https://matched/%s/%s", orgName, repoName),
300+
setupMux: func(mux *http.ServeMux, jwtToken string) {
301+
mux.HandleFunc(fmt.Sprintf("/repos/%s/%s/installation", orgName, repoName), func(w http.ResponseWriter, _ *http.Request) {
302+
_, _ = fmt.Fprintf(w, `{"id": %d}`, orgID)
303+
})
304+
mux.HandleFunc(fmt.Sprintf("/app/installations/%d/access_tokens", orgID), func(w http.ResponseWriter, r *http.Request) {
305+
testMethod(t, r)
306+
w.Header().Set("Authorization", "Bearer "+jwtToken)
307+
w.Header().Set("Accept", "application/vnd.github+json")
308+
_, _ = fmt.Fprintf(w, `{"token": "%s"}`, wantToken)
309+
})
310+
},
311+
wantErr: false,
312+
wantInstallationID: orgID,
313+
wantToken: wantToken,
314+
},
297315
{
298316
name: "repo installation fails, org installation succeeds",
299317
repoURL: fmt.Sprintf("https://matched/%s/%s", orgName, repoName),
@@ -357,13 +375,40 @@ func TestGetAndUpdateInstallationID_Fallbacks(t *testing.T) {
357375
expectedErrorString: "could not find repository, organization or user installation",
358376
},
359377
{
360-
name: "invalid repo url",
361-
repoURL: "https://invalid/url",
362-
setupMux: func(_ *http.ServeMux, _ string) {
378+
name: "repo installation succeeds but token generation fails",
379+
repoURL: fmt.Sprintf("https://matched/%s/%s", orgName, repoName),
380+
setupMux: func(mux *http.ServeMux, jwtToken string) {
381+
mux.HandleFunc(fmt.Sprintf("/repos/%s/%s/installation", orgName, repoName), func(w http.ResponseWriter, _ *http.Request) {
382+
_, _ = fmt.Fprintf(w, `{"id": %d}`, orgID)
383+
})
384+
mux.HandleFunc(fmt.Sprintf("/app/installations/%d/access_tokens", orgID), func(w http.ResponseWriter, r *http.Request) {
385+
testMethod(t, r)
386+
w.Header().Set("Authorization", "Bearer "+jwtToken)
387+
w.Header().Set("Accept", "application/vnd.github+json")
388+
w.WriteHeader(http.StatusForbidden)
389+
_, _ = fmt.Fprintf(w, `{"message": "Forbidden"}`)
390+
})
363391
},
364-
wantErr: true,
365-
expectedErrorString: "invalid repository URL path",
392+
wantErr: false, // Should not error, but return empty token
393+
wantInstallationID: orgID,
394+
wantToken: "", // Empty token when generation fails
366395
},
396+
{
397+
name: "invalid repo url - too few path segments",
398+
repoURL: "https://invalid/url",
399+
setupMux: func(_ *http.ServeMux, _ string) {
400+
},
401+
wantErr: true,
402+
expectedErrorString: "invalid repository URL path",
403+
},
404+
{
405+
name: "invalid GitHub repo url - too many path segments",
406+
repoURL: "https://github.com/group/subgroup/repo",
407+
setupMux: func(_ *http.ServeMux, _ string) {
408+
},
409+
wantErr: true,
410+
expectedErrorString: "invalid repository URL path",
411+
},
367412
}
368413

369414
for _, tt := range tests {
@@ -393,7 +438,7 @@ func TestGetAndUpdateInstallationID_Fallbacks(t *testing.T) {
393438
ctx = info.StoreCurrentControllerName(ctx, "default")
394439
ctx = info.StoreNS(ctx, testNamespace.GetName())
395440

396-
ip := NewInstallation(httptest.NewRequest(http.MethodGet, "http://localhost", strings.NewReader("")), run, &v1alpha1.Repository{}, &github.Provider{}, testNamespace.GetName())
441+
ip := NewInstallation(httptest.NewRequest(http.MethodGet, "http://localhost", strings.NewReader("")), run, &v1alpha1.Repository{}, &github.Provider{}, testNamespace.GetName(), nil)
397442
jwtToken, err := ip.GenerateJWT(ctx)
398443
assert.NilError(t, err)
399444

@@ -412,7 +457,7 @@ func TestGetAndUpdateInstallationID_Fallbacks(t *testing.T) {
412457
gprovider.SetGithubClient(fakeghclient)
413458
t.Setenv("PAC_GIT_PROVIDER_TOKEN_APIURL", serverURL)
414459

415-
ip = NewInstallation(httptest.NewRequest(http.MethodGet, "http://localhost", strings.NewReader("")), run, repo, gprovider, testNamespace.GetName())
460+
ip = NewInstallation(httptest.NewRequest(http.MethodGet, "http://localhost", strings.NewReader("")), run, repo, gprovider, testNamespace.GetName(), nil)
416461
_, token, installationID, err := ip.GetAndUpdateInstallationID(ctx)
417462

418463
if tt.wantErr {

0 commit comments

Comments
 (0)