Conversation
Signed-off-by: Smyslov Maxim <maksim.smyslov@flant.com>
ldmonster
reviewed
Mar 20, 2026
Signed-off-by: Smyslov Maxim <maksim.smyslov@flant.com>
Signed-off-by: Smyslov Maxim <maksim.smyslov@flant.com>
Signed-off-by: Smyslov Maxim <maksim.smyslov@flant.com>
Signed-off-by: Smyslov Maxim <maksim.smyslov@flant.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR increases logging verbosity and consistency across the webhook stack by injecting the shell-operator logger through webhook managers/handlers/server, improving structured fields, and wrapping errors to add context for easier debugging.
Changes:
- Add logger plumbing via constructors/options for webhook server, managers, handlers, and webhook resources.
- Rewrite/augment log messages with additional structured fields; wrap returned errors with context.
- Add explicit 400 responses for invalid webhook requests; fix tests to initialize shell-operator with a logger.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test/hook/context/context_combiner.go | Update test helper to construct ShellOperator via constructor and provide a nop logger/metric storage to avoid nil logger deref. |
| pkg/webhook/server/server_test.go | Update test to use NewWebhookServer with a nop logger. |
| pkg/webhook/server/server.go | Add WebhookServer.Logger and NewWebhookServer constructor; use injected logger in Start(). |
| pkg/webhook/conversion/manager.go | Add logger option/default; forward logger to handler/server; wrap errors with context. |
| pkg/webhook/conversion/handler.go | Use injected logger; add request-scoped fields; add 400 responses for invalid input; wrap handler errors. |
| pkg/webhook/conversion/crd_client_config.go | Wrap CRD get/update errors with context. |
| pkg/webhook/admission/resource.go | Inject logger into resources; improve logging and wrap list errors. |
| pkg/webhook/admission/manager.go | Add logger option/default; forward logger to handler/server/resources; wrap errors with context. |
| pkg/webhook/admission/handler.go | Use injected logger; add request-scoped fields; add 400 responses for invalid input; wrap handler errors. |
| pkg/shell-operator/operator.go | Switch some logs to op.logger; adjust admission event logging fields; tweak conversion failure message. |
| pkg/shell-operator/bootstrap.go | Pass shell-operator logger into admission/conversion webhook managers. |
| pkg/hook/hook_manager.go | Replace global logging with manager logger and improve structured logging fields. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ipaqsa
approved these changes
Mar 23, 2026
ldmonster
approved these changes
Mar 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Increased logs verbosity.
Changes made:
What this PR does / why we need it
These changes are necessary to make logs easier to read and improve debugging.
Special notes for your reviewer