Skip to content

Commit e1a2f48

Browse files
KMI1011theakshaypant
authored andcommitted
fix: downgrade 404 API responses from error to debug log level
Replace duplicate 404 and non-404 error subtests with a table-driven test to satisfy the dupl linter. Fixes #2653 rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
1 parent 74939ef commit e1a2f48

2 files changed

Lines changed: 99 additions & 72 deletions

File tree

pkg/provider/github/profiler.go

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,8 @@ func (v *Provider) checkRateLimit(resp *github.Response) (remaining string) {
7676
if v.eventEmitter != nil {
7777
v.eventEmitter.EmitMessage(
7878
v.repo, zap.ErrorLevel, "GitHubRateLimitExceeded",
79-
fmt.Sprintf("GitHub API rate limit exceeded, limit: %s, resets at: %s", limit, reset))
79+
fmt.Sprintf("GitHub API rate limit exceeded, limit: %s, resets at: %s", limit, reset),
80+
)
8081
}
8182
case remainingCount < rateLimitCritical:
8283
v.Logger.Errorw("GitHub API rate limit critically low", logFields...)
@@ -123,7 +124,8 @@ func (v *Provider) logAPICall(operation string, duration time.Duration, resp *gi
123124
// Add response context if available
124125
if resp != nil {
125126
remaining := v.checkRateLimit(resp)
126-
logFields = append(logFields,
127+
logFields = append(
128+
logFields,
127129
"url_path", resp.Request.URL.Path,
128130
"rate_limit_remaining", remaining,
129131
"github_request_id", resp.Header.Get("X-GitHub-Request-Id"),
@@ -132,11 +134,15 @@ func (v *Provider) logAPICall(operation string, duration time.Duration, resp *gi
132134
logFields = append(logFields, "status_code", resp.StatusCode)
133135
}
134136
}
135-
136-
// Log based on success/failure with appropriate level
137+
// Log success/failure appropriately; 404 is debug-only
138+
// since a missing OWNERS file, for example, is a valid/expected state
137139
if err != nil {
138140
logFields = append(logFields, "error", err.Error())
139-
v.Logger.Errorw("GitHub API call failed", logFields...)
141+
if resp != nil && resp.Response != nil && resp.StatusCode == http.StatusNotFound {
142+
v.Logger.Debugw("GitHub API call returned not found", logFields...)
143+
} else {
144+
v.Logger.Errorw("GitHub API call failed", logFields...)
145+
}
140146
} else {
141147
v.Logger.Debugw("GitHub API call completed", logFields...)
142148
}

pkg/provider/github/profiler_test.go

Lines changed: 88 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -33,80 +33,100 @@ func TestWrapAPI(t *testing.T) {
3333
assert.Assert(t, err == nil)
3434
})
3535

36-
// Test case: Logger is not nil
37-
t.Run("Logger is not nil", func(t *testing.T) {
38-
observedZapCore, observedLogs := observer.New(zap.DebugLevel)
39-
observedLogger := zap.New(observedZapCore).Sugar()
40-
p := &Provider{
41-
Logger: observedLogger,
42-
providerName: "github",
43-
triggerEvent: "pull_request",
44-
repo: &v1alpha1.Repository{
45-
ObjectMeta: metav1.ObjectMeta{
46-
Namespace: "test-ns",
47-
Name: "test-repo",
36+
errorLogTests := []struct {
37+
name string
38+
statusCode int
39+
expectedMsg string
40+
expectedLevel zapcore.Level
41+
}{
42+
{
43+
name: "API 404 logs at debug level",
44+
statusCode: http.StatusNotFound,
45+
expectedMsg: "GitHub API call returned not found",
46+
expectedLevel: zap.DebugLevel,
47+
},
48+
{
49+
name: "non-404 error logs at error level",
50+
statusCode: http.StatusForbidden,
51+
expectedMsg: "GitHub API call failed",
52+
expectedLevel: zap.ErrorLevel,
53+
},
54+
}
55+
for _, tt := range errorLogTests {
56+
t.Run(tt.name, func(t *testing.T) {
57+
observedZapCore, observedLogs := observer.New(zap.DebugLevel)
58+
observedLogger := zap.New(observedZapCore).Sugar()
59+
p := &Provider{
60+
Logger: observedLogger,
61+
providerName: "github",
62+
triggerEvent: "pull_request",
63+
repo: &v1alpha1.Repository{
64+
ObjectMeta: metav1.ObjectMeta{
65+
Namespace: "test-ns",
66+
Name: "test-repo",
67+
},
4868
},
49-
},
50-
}
51-
called := false
52-
reqURL, _ := url.Parse("https://api.github.com/test")
53-
headers := http.Header{}
54-
headers.Set("X-RateLimit-Remaining", "99")
55-
resp := &github.Response{
56-
Response: &http.Response{
57-
Request: &http.Request{URL: reqURL},
58-
Header: headers,
59-
},
60-
}
61-
call := func() (string, *github.Response, error) {
62-
called = true
63-
return "data", resp, fmt.Errorf("error")
64-
}
69+
}
70+
called := false
71+
reqURL, _ := url.Parse("https://api.github.com/test")
72+
headers := http.Header{}
73+
headers.Set("X-RateLimit-Remaining", "99")
74+
resp := &github.Response{
75+
Response: &http.Response{
76+
StatusCode: tt.statusCode,
77+
Request: &http.Request{URL: reqURL},
78+
Header: headers,
79+
},
80+
}
81+
call := func() (string, *github.Response, error) {
82+
called = true
83+
return "data", resp, fmt.Errorf("error")
84+
}
6585

66-
data, r, e := wrapAPI(p, "test_api_call", call)
86+
data, r, e := wrapAPI(p, "test_api_call", call)
6787

68-
assert.Assert(t, called)
69-
assert.Equal(t, "data", data)
70-
assert.Equal(t, r, resp)
71-
assert.Error(t, e, "error")
88+
assert.Assert(t, called)
89+
assert.Equal(t, "data", data)
90+
assert.Equal(t, r, resp)
91+
assert.Error(t, e, "error")
7292

73-
logs := observedLogs.All()
74-
assert.Assert(t, len(logs) > 0, "Should have log entries")
93+
logs := observedLogs.All()
94+
assert.Assert(t, len(logs) > 0, "Should have log entries")
7595

76-
// Find the API call log entry
77-
var apiCallLog *observer.LoggedEntry
78-
for i := range logs {
79-
if logs[i].Message == "GitHub API call failed" {
80-
apiCallLog = &logs[i]
81-
break
96+
var apiCallLog *observer.LoggedEntry
97+
for i := range logs {
98+
if logs[i].Message == tt.expectedMsg {
99+
apiCallLog = &logs[i]
100+
break
101+
}
82102
}
83-
}
84-
assert.Assert(t, apiCallLog != nil, "Should have API call failed log entry")
85-
86-
// Check structured fields
87-
foundOperation := false
88-
foundProvider := false
89-
foundRepo := false
90-
for _, field := range apiCallLog.Context {
91-
switch field.Key {
92-
case "operation":
93-
assert.Equal(t, "test_api_call", field.String)
94-
foundOperation = true
95-
case "provider":
96-
assert.Equal(t, "github", field.String)
97-
foundProvider = true
98-
case "repo":
99-
assert.Equal(t, "test-ns/test-repo", field.String)
100-
foundRepo = true
103+
assert.Assert(t, apiCallLog != nil, "Should have log entry: %s", tt.expectedMsg)
104+
assert.Equal(t, tt.expectedLevel, apiCallLog.Level)
105+
106+
foundOperation := false
107+
foundProvider := false
108+
foundRepo := false
109+
for _, field := range apiCallLog.Context {
110+
switch field.Key {
111+
case "operation":
112+
assert.Equal(t, "test_api_call", field.String)
113+
foundOperation = true
114+
case "provider":
115+
assert.Equal(t, "github", field.String)
116+
foundProvider = true
117+
case "repo":
118+
assert.Equal(t, "test-ns/test-repo", field.String)
119+
foundRepo = true
120+
}
101121
}
102-
}
103122

104-
assert.Assert(t, foundOperation, "Should have operation field")
105-
assert.Assert(t, foundProvider, "Should have provider field")
106-
assert.Assert(t, foundRepo, "Should have repo field")
107-
})
123+
assert.Assert(t, foundOperation, "Should have operation field")
124+
assert.Assert(t, foundProvider, "Should have provider field")
125+
assert.Assert(t, foundRepo, "Should have repo field")
126+
})
127+
}
108128

109-
// Test case: Logger is not nil and response is nil
129+
// Nil response omits URL and rate-limit fields from the log entry.
110130
t.Run("Logger is not nil, response is nil", func(t *testing.T) {
111131
observedZapCore, observedLogs := observer.New(zap.DebugLevel)
112132
observedLogger := zap.New(observedZapCore).Sugar()
@@ -292,8 +312,9 @@ func TestWrapGetContents(t *testing.T) {
292312
headers.Set("X-RateLimit-Remaining", "42")
293313
resp := &github.Response{
294314
Response: &http.Response{
295-
Request: &http.Request{URL: reqURL},
296-
Header: headers,
315+
StatusCode: http.StatusForbidden,
316+
Request: &http.Request{URL: reqURL},
317+
Header: headers,
297318
},
298319
}
299320
fileContent := &github.RepositoryContent{Name: github.Ptr("file")}

0 commit comments

Comments
 (0)