feat: remove logrus and propogate context everywhere#18
Closed
Joibel wants to merge 3 commits into
Closed
Conversation
Joibel
force-pushed
the
remove-logrus
branch
2 times, most recently
from
July 18, 2025 16:20
8665987 to
0c4c916
Compare
Owner
Author
|
cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
This PR removes the logrus logging library and propagates Go context throughout the codebase. The purpose is to modernize the logging infrastructure and improve context handling across the workflow controller.
- Remove logrus dependency and replace with consistent context-based logging
- Propagate context parameter through function signatures and method calls
- Update logging patterns to use context-aware logging methods
Reviewed Changes
Copilot reviewed 300 out of 397 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| workflow/controller/operator_agent_test.go | Updated test setup to use TestContext and propagate context to controller initialization |
| workflow/controller/operator.go | Replaced logrus imports, updated logging calls to be context-aware, and propagated context through method signatures |
| workflow/controller/ns_watcher.go | Updated namespace watcher logging to use RequireLoggerFromContext and context-aware logging calls |
| workflow/controller/node_counters_test.go | Updated test setup to use TestContext instead of manual context creation |
| workflow/controller/inline_test.go | Simplified test context setup using TestContext helper |
| workflow/controller/informer/workflow_template_convert.go | Added context parameter to Get method signature |
| workflow/controller/informer/cluster_workflow_template_convert.go | Added context parameter to Get method signature |
| workflow/controller/indexes/workflow_index.go | Replaced logrus with context-aware logging in package initialization |
| workflow/controller/http_template_test.go | Simplified test context setup |
| workflow/controller/hooks_test.go | Updated all test functions to use TestContext helper |
| workflow/controller/hooks.go | Updated function signatures to use TemplateContext instead of Context |
| workflow/controller/healthz_test.go | Added TestContext usage and LogMiddleware for HTTP handler |
| workflow/controller/healthz.go | Replaced logrus with context-aware logging and added LogMiddleware |
| workflow/controller/exit_handler_test.go | Updated test functions to use TestContext helper |
| workflow/controller/exit_handler.go | Updated function signatures and context-aware logging calls |
Comments suppressed due to low confidence (2)
workflow/controller/operator.go:1936
- The type name has changed from
templateresolution.Contexttotemplateresolution.TemplateContext. Ensure this naming change is consistent across the codebase.
func (woc *wfOperationCtx) executeTemplate(ctx context.Context, nodeName string, orgTmpl wfv1.TemplateReferenceHolder, tmplCtx *templateresolution.TemplateContext, args wfv1.Arguments, opts *executeTemplateOpts) (node *wfv1.NodeStatus, err error) {
| // for before requeuing the workflow onto the workqueue. | ||
| var ( | ||
| maxOperationTime = envutil.LookupEnvDurationOr(logging.WithLogger(context.Background(), logging.NewSlogLogger(logging.GetGlobalLevel(), logging.GetGlobalFormat())), "MAX_OPERATION_TIME", 30*time.Second) | ||
| maxOperationTime = envutil.LookupEnvDurationOr(logging.InitLoggerInContext(), "MAX_OPERATION_TIME", 30*time.Second) |
There was a problem hiding this comment.
[nitpick] Using InitLoggerInContext() for environment variable lookup seems inconsistent. Consider using a dedicated context for configuration loading rather than a logger-initialized context.
| }() | ||
| labelSelector := "!" + common.LabelKeyPhase + "," + instanceIDSelector | ||
| err := func() error { | ||
| err := func(ctx context.Context) error { |
There was a problem hiding this comment.
[nitpick] The nested function definition could be extracted to improve readability and testability.
Joibel
force-pushed
the
remove-logrus
branch
6 times, most recently
from
July 21, 2025 10:17
d108012 to
9b768cf
Compare
Joibel
force-pushed
the
remove-logrus
branch
8 times, most recently
from
July 23, 2025 12:57
af71e05 to
982ea6b
Compare
Owner
Author
|
cursor review |
Joibel
force-pushed
the
remove-logrus
branch
4 times, most recently
from
July 28, 2025 09:04
df8f2de to
e7f6cac
Compare
Signed-off-by: Alan Clucas <alan@clucas.org>
Signed-off-by: Alan Clucas <alan@clucas.org>
Signed-off-by: Alan Clucas <alan@clucas.org>
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.
Practice PR to see what's borked