feat: Add response and log functionalities#603
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds reusable helpers for HTTP error responses and introduces a public logging utility package so external consumers can integrate with Microcluster’s logging/context mechanism.
Changes:
- Added
UnauthorizedandErrorResponsehelpers inmicrocluster/rest/response/response.gofor standardized 401 and arbitrary-code error responses. - Introduced
pkg/log/log.goto expose the logger context key (CtxLogger) and aWithLoggerhelper that stores aslog.Loggerin acontext.Context.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/log/log.go | New public logging helper package that wraps the internal logger context key and provides WithLogger for attaching a logger to a context. |
| microcluster/rest/response/response.go | Extends the HTTP response helpers with 401 and generic error-response constructors and wires in errors.New for message-based error responses. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
7c09f3b to
e30187f
Compare
e30187f to
1af6e75
Compare
roosterfish
left a comment
There was a problem hiding this comment.
Thanks for adding the two additional response funcs. However I still don't see the need for the proposed logging changes.
|
Thank you for the review @roosterfish. I've reached out on MM with more context around this change. |
Ty for providing the additional context. So as part of our unit tests we also run Deriving another default logger on the |
1af6e75 to
d7202ad
Compare
|
Thank you for all the information @roosterfish! I've updated the PR with what we've discussed |
e0ae35a to
e49501f
Compare
|
Can you please rebase, there are now conflicts after merging in other PRs. |
c250354 to
e81370e
Compare
e81370e to
dde6ba3
Compare
|
@roosterfish Thank you for the review. I've updated the PR with your instructions. Please let me know if it needs further polishing. |
roosterfish
left a comment
There was a problem hiding this comment.
The current logger changes feel too invasive to me.
In one of the recent comments I mentioned the approach we take for logging.
I understand you want to have an easy way to derive a context with the logger set to mimic what is done internally by Microcluster so you can test e.g. API handlers as standalone (as we do in our unit tests).
The easiest (which already works today) is to derive a context on your own using context.WithValue(context.TODO(), log.CtxLogger, slog.Default()).
But together we identified that log.CtxLogger is in the internal/ package. To overcome this limitation my suggestion was to move this constant into microcluster/types/log instead so you can access it too. It can even be a just a file like log.go (no need for an extra log/ package).
But this should not require this amount of changes. Only the places where we currently use log.CtxLogger need to be updated to point to the micrcluster/types package which would then contain the constant.
|
If you intend to have a little helper func, you can keep the |
3f6125c to
c1dfed7
Compare
c1dfed7 to
a154751
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ons for improved error handling Signed-off-by: Homayoon Alimohammadi <homayoon.alimohammadi@canonical.com>
…zed logger type Signed-off-by: Homayoon Alimohammadi <homayoon.alimohammadi@canonical.com>
a154751 to
57fa40f
Compare
Context
While migrating Canonical Kubernetes from Microcluster v2 to v3, I noticed that we're moving away from the
lxd/reponseand it's now baked into Microcluster itself. These two functions were used in Canonical Kubernetes but are not available in Microcluster. Also, the logging package is internal to Microcluster which makes testing on the k8s snap side impossible. The newpkg/logis supposed to surface the logging functionality.Overview
This pull request introduces new functionality for error handling in HTTP responses and adds a new logging utility package. The most important changes are the addition of helper functions for standardized error responses and the creation of a
logpackage to simplify logger usage with context.Error handling improvements:
UnauthorizedandErrorResponsehelper functions toresponse.gofor generating 401 and custom error responses, respectively.Logging utility:
microcluster/types/logpackage that wraps logger context handling, providing aWithLoggerfunction and exposing the logger context key for easier logger management from external repos.