fix(logging): log sinker errors , emit event on repository config error - #2848
fix(logging): log sinker errors , emit event on repository config error#2848aThorp96 wants to merge 1 commit into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2848 +/- ##
==========================================
+ Coverage 61.62% 61.67% +0.05%
==========================================
Files 213 213
Lines 17512 17523 +11
==========================================
+ Hits 10792 10808 +16
- Misses 5901 5902 +1
+ Partials 819 813 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new sentinel error ErrSecretNotFound in the secrets package and wraps repository secret retrieval errors with it. In the event sinker, if client setup fails due to a missing repository secret, a Kubernetes event is now emitted to notify the user. Additionally, a wrapper method handleEvent was added to centralize event processing error logging, and comprehensive unit tests were added to verify the new error handling behavior. There are no review comments, so I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Since `sinker.logger` is modified during `sinker.processEvent()`, if an error is returned from `processEvent()` it lacks these modifications (i.e. added context, like repository URL or commit sha) This change also improves the logging when PaC fails to process an event due to a misconfigured Repository CR. Specifically, when the required GitProvider.Secret is either unspecified or missing PaC will now emit an Event on the Repository CR. Events are a clearer signal to users than logs since tenants may be given access to events in their namespace whereas PaC logs are considered more sensitive
9bda827 to
73b9368
Compare
|
/retest |
| // If setup fails here, it's a configuration error and we should fail fast | ||
| if err := s.setupClient(ctx, repo); err != nil { | ||
| if errors.Is(err, secrets.ErrSecretNotFound) { | ||
| events.NewEventEmitter(s.run.Clients.Kube, s.logger).EmitMessage( |
There was a problem hiding this comment.
EmitMessage also logs the message along with creating an event so an error here would be logged twice, once here and once again at handleEvent
| return nil | ||
| } | ||
|
|
||
| func (s *sinker) handleEvent(ctx context.Context, request *http.Request) error { |
There was a problem hiding this comment.
what's the purpose of this func? before it was done in same way i.e. logging error on err is not nil
There was a problem hiding this comment.
I updated the PR description to be slightly clearer. Logging using the sinkers logger is different than logging using the adapter's logger.
Since sinker.logger is modified during sinker.processEvent(), logging the error from inside the sinker retains any logger context parameters. For example the code path where sinker.processEventPayload() succeeds (and modifies the logger), but sinker.setupClient() returns an error.
When investigating "why did PaC fail to start a pipelineRun for this commit", best practice is to search the PaC logs for event-sha=<commit-sha> and/or source-repo-url=<repo>. In the above example, before this change, the sinker sets event-sha and source-repo-urlon the logger but the error log is logged by the local-variable logger in listener.handleEvent so it doesn't contain any of the parsed log tags.
There was a problem hiding this comment.
Pull request overview
This pull request improves observability around webhook/event processing by ensuring errors are logged with the enriched sinker logger context, and by emitting Kubernetes Events on Repository resources when event processing fails due to repository secret misconfiguration.
Changes:
- Introduces
secrets.ErrSecretNotFoundto classify certain repository secret/config errors and updates error strings accordingly. - Routes adapter error logging through
sinker.handleEvent()so error logs use the sinker’s enriched logger fields. - Emits a
RepositorySecretMissingEvent on the Repository CR when client setup fails due toErrSecretNotFound, plus adds unit tests for secret misconfiguration cases.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/secrets/secret.go | Adds ErrSecretNotFound and wraps specific repository secret/config failures to enable upstream handling. |
| pkg/secrets/secret_test.go | Adds unit tests validating the new secret-misconfiguration error behavior. |
| pkg/pipelineascode/match_test.go | Updates expected error message text due to new wrapped error prefix. |
| pkg/adapter/sinker.go | Adds handleEvent() wrapper logging and emits a Repository Event when secret misconfig prevents client setup. |
| pkg/adapter/adapter.go | Switches goroutine path to call s.handleEvent() instead of s.processEvent() and removes duplicate outer error logging. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Namespace: s.Namespace, | ||
| Name: s.Repo.Spec.GitProvider.Secret.Name, | ||
| Key: gitProviderSecretKey, | ||
| }); err != nil { | ||
| return err | ||
| return fmt.Errorf("%w: error getting provider secret: %w", ErrSecretNotFound, err) |
| Namespace: s.Namespace, | ||
| Name: s.Repo.Spec.GitProvider.WebhookSecret.Name, | ||
| Key: gitProviderWebhookSecretKey, | ||
| }); err != nil { | ||
| return err | ||
| return fmt.Errorf("%w: error getting webhook secret: %w", ErrSecretNotFound, err) |
| stdata, _ := testclient.SeedTestData(t, ctx, tdata) | ||
| run := ¶ms.Run{ |
|
/paco review |
|
@aThorp96 can you please update the PR? |
|
@zakisk what about pushing fixes on PR directly instead of waiting to ask, andrew is pretty busy on a bunch of stuff |
📝 Description of the Change
Since
sinker.loggeris modified duringsinker.processEvent(), if an error is returned fromprocessEvent()logging from the adapter logs without these modifications (i.e. the log won't have added context like repository URL, commit sha, etc)This change also improves the logging when PaC fails to process an event due to a misconfigured Repository CR. Specifically, when the required GitProvider.Secret is either unspecified or missing PaC will now emit an Event on the Repository CR. Events are a clearer signal to users than logs since tenants may be given access to events in their namespace whereas PaC logs are considered more sensitive
🔗 Linked GitHub Issue
Fixes #
🧪 Testing Strategy
🤖 AI Assistance
AI assistance can be used for various tasks, such as code generation,
documentation, or testing.
Please indicate whether you have used AI assistance
for this PR and provide details if applicable.
Important
Slop will be simply rejected, if you are using AI assistance you need to make sure you
understand the code generated and that it meets the project's standards. you
need at least know how to run the code and deploy it (if needed). See
startpaac to make it easy
to deploy and test your code changes.
If the majority of the code in this PR was generated by an AI, please add a
Co-authored-bytrailer to your commit message.For example:
Co-authored-by: Claude noreply@anthropic.com
✅ Submitter Checklist
fix:,feat:) matches the "Type of Change" I selected above.make testandmake lintlocally to check for and fix anyissues. For an efficient workflow, I have considered installing
pre-commit and running
pre-commit installtoautomate these checks.