Remove fmt.Println, convert to log #3718
Open
Conversation
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
Signed-off-by: Jason Parraga <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR refactors logging by removing direct fmt.Println/Printf calls and converting them into structured logging calls, while also updating several utility functions to accept a context parameter for better consistency. Key changes include:
- Replacing fmt-based output in utilities with ctx-aware log.Info calls.
- Updating function signatures (e.g. CheckName, CheckLabel, TrimJobName) and corresponding test cases to require an explicit context.
- Propagating context through various controllers and helpers to improve logging and maintain consistency.
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| ray-operator/controllers/ray/utils/util_test.go | Updated test to pass context when calling CheckName |
| ray-operator/controllers/ray/utils/util.go | Modified CheckName to accept context and log infos |
| ray-operator/controllers/ray/suite_helpers_test.go | Removed fmt.Println calls in favor of structured logging |
| ray-operator/controllers/ray/raycluster_controller_unit_test.go | Converted CheckLabel calls to include context |
| ray-operator/controllers/ray/raycluster_controller.go | Passed context to utility functions within reconcilers |
| ray-operator/controllers/ray/common/test_utils.go | Updated shortString to receive context |
| ray-operator/controllers/ray/common/service_test.go | Passed context into HeadServiceLabels in tests |
| ray-operator/controllers/ray/common/service.go | Updated function signatures to include ctx, improved logging |
| ray-operator/controllers/ray/common/route_test.go | Modified BuildRouteForHeadService call to pass context |
| ray-operator/controllers/ray/common/route.go | Updated BuildRouteForHeadService to take context |
| ray-operator/controllers/ray/common/rbac_test.go | Passed context to BuildRoleBinding within tests |
| ray-operator/controllers/ray/common/rbac.go | Updated BuildRoleBinding function to accept context |
| ray-operator/controllers/ray/common/pod_test.go | Updated test assertions to use context-aware utilities |
| ray-operator/controllers/ray/common/pod.go | Passed context into CheckName and labelPod calls |
| ray-operator/controllers/ray/common/ingress.go | Passed context argument to CheckLabel |
| ray-operator/controllers/ray/common/association_test.go | Updated HeadServiceLabels usage to include context |
| ray-operator/controllers/ray/common/association.go | Updated RayClusterHeadServiceListOptions to require context |
Comments suppressed due to low confidence (1)
ray-operator/controllers/ray/common/pod.go:187
- Verify that the context propagation and usage in utility functions like CheckName is consistent across similar code paths to ensure uniform logging across the codebase.
podTemplate.Spec.ServiceAccountName = utils.CheckName(ctx, utils.GetHeadGroupServiceAccountName(&instance))
| maxLength := 50 // 63 - (max(8,6) + 5 ) // 6 to 8 char are consumed at the end with "-head-" or -worker- + 5 generated. | ||
|
|
||
| if len(s) > maxLength { | ||
| // shorten the name | ||
| offset := int(math.Abs(float64(maxLength) - float64(len(s)))) | ||
| fmt.Printf("pod name is too long: len = %v, we will shorten it by offset = %v", len(s), offset) | ||
| log.Info("pod name is too long, we will shorten it by offset", "nameLength", len(s), "offset", offset) |
There was a problem hiding this comment.
[nitpick] Consider including additional context—in particular, the truncated segment or original name length—in the log message to aid in debugging in production.
Suggested change
| log.Info("pod name is too long, we will shorten it by offset", "nameLength", len(s), "offset", offset) | |
| log.Info("pod name is too long, we will shorten it by offset", "originalName", s, "truncatedSegment", s[:offset], "nameLength", len(s), "offset", offset) |
Copilot uses AI. Check for mistakes.
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.
Why are these changes needed?
I work on some code that integrates with kuberay and we noticed some printlns in the output. I have removed unnecessary printlns and moved useful ones to use the logger so the output will be uniform.
Related issue number
Checks