refactor(eventrecorder): rename EventRecorderConfig/Output and introduce output-type constants#5190
Merged
Merged
Conversation
📝 WalkthroughWalkthroughRename/refactor eventrecorder config types ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
eventrecorder/eventrecorder.go (1)
46-49: Optional: consider untyped constants or a dedicated output-type string type.Declaring
OutputFile string = "file"ties each constant to the concretestringtype. Either making them untyped (OutputFile = "file") or introducing a dedicated alias (type OutputType string) is more idiomatic and, in the latter case, lets the compiler catch bare string comparisons againstOutput.Type. SinceOutput.Typeis a plainstringtoday, this is purely a nit.♻️ Option A: untyped string constants
const ( - OutputFile string = "file" - OutputWebhook string = "webhook" + OutputFile = "file" + OutputWebhook = "webhook" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eventrecorder/eventrecorder.go` around lines 46 - 49, Change the typed string constants OutputFile and OutputWebhook to either untyped constants or introduce a dedicated alias type to make intent explicit: replace the current declarations of OutputFile and OutputWebhook and, if you choose the alias approach, add type OutputType string and update any uses (e.g., the Output.Type field and comparisons against OutputFile/OutputWebhook) so the field and constants use OutputType instead of plain string; otherwise simply remove the explicit `string` type on the two constants to make them untyped.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@eventrecorder/eventrecorder.go`:
- Around line 46-49: Change the typed string constants OutputFile and
OutputWebhook to either untyped constants or introduce a dedicated alias type to
make intent explicit: replace the current declarations of OutputFile and
OutputWebhook and, if you choose the alias approach, add type OutputType string
and update any uses (e.g., the Output.Type field and comparisons against
OutputFile/OutputWebhook) so the field and constants use OutputType instead of
plain string; otherwise simply remove the explicit `string` type on the two
constants to make them untyped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9bcce745-d430-4543-883d-f15d5d40bb80
📒 Files selected for processing (6)
config/config.goeventrecorder/config.goeventrecorder/eventrecorder.goeventrecorder/eventrecorder_test.goeventrecorder/webhook.goeventrecorder/webhook_test.go
EventRecorderConfig / EventRecorderOutput repeat the package name. Idiomatic golang would be to export as Config and Output so callers write eventrecorder.Config instead. Signed-off-by: emreya <e.yazici1990@gmail.com>
This makes the switch in UnmarshalYAML and buildOutputs resilient to typos. Signed-off-by: emreya <e.yazici1990@gmail.com>
…type Closer to config struct declaration, in case of new field changes, changes are collocated in single file. Signed-off-by: emreya <e.yazici1990@gmail.com>
Declaring string type is unnecessary for enum consts. Signed-off-by: emreya <e.yazici1990@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (3)
config/config.go (1)
222-229: Use the exportedeventrecorder.OutputFileconstant.This is the same string-literal duplication this PR is trying to eliminate. Replacing
"file"here witheventrecorder.OutputFilekeeps the type comparison resilient to typos and consistent withbuildOutputsandUnmarshalYAML.♻️ Proposed change
for i, out := range cfg.EventRecorder.Outputs { - if out.Type == "file" { + if out.Type == eventrecorder.OutputFile { cfg.EventRecorder.Outputs[i].Path = join(out.Path) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/config.go` around lines 222 - 229, Replace the string literal "file" in the loop that processes cfg.EventRecorder.Outputs with the exported constant eventrecorder.OutputFile to avoid duplication and typos; specifically, in the loop iterating over cfg.EventRecorder.Outputs where you check out.Type and call cfg.EventRecorder.Outputs[i].Path = join(out.Path), change the comparison to use eventrecorder.OutputFile so it matches the usage in buildOutputs and UnmarshalYAML and keeps behavior identical while relying on the shared constant.eventrecorder/config.go (1)
82-84: Optional: rename helper to match the renamed type.The renaming refactor dropped the
EventRecorderprefix from the types, but the helper name still keeps the long form. Since this is unexported and used only within the package, considerconfigEqual(orconfigsEqual) for consistency withConfig/Output.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eventrecorder/config.go` around lines 82 - 84, The helper function name eventRecorderConfigEqual is inconsistent with the refactored Config/Output types; rename this unexported helper to a shorter, package-consistent name such as configEqual (or configsEqual) and update all internal references to eventRecorderConfigEqual accordingly so the function signature and usages match the new name while preserving its logic for comparing Config values.eventrecorder/eventrecorder_test.go (1)
237-246: Use the newOutputFileconstant in tests for consistency.The PR introduces
OutputFile/OutputWebhookto remove duplicated string literals, but these test fixtures still hardcode"file". Switching to the constant aligns the tests with the refactor's goal.♻️ Proposed change
a := Config{ Outputs: []Output{ - {Type: "file", Path: "/tmp/events.jsonl"}, + {Type: OutputFile, Path: "/tmp/events.jsonl"}, }, } b := Config{ Outputs: []Output{ - {Type: "file", Path: "/tmp/events.jsonl"}, + {Type: OutputFile, Path: "/tmp/events.jsonl"}, }, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eventrecorder/eventrecorder_test.go` around lines 237 - 246, Tests still use the hardcoded "file" string for outputs; update the two Output entries in the test fixtures (the Config/Output structs named a and b) to use the new OutputFile constant instead of the literal "file" so the tests align with the refactor (replace {Type: "file", ...} with {Type: OutputFile, ...} in both places).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@config/config.go`:
- Around line 222-229: Replace the string literal "file" in the loop that
processes cfg.EventRecorder.Outputs with the exported constant
eventrecorder.OutputFile to avoid duplication and typos; specifically, in the
loop iterating over cfg.EventRecorder.Outputs where you check out.Type and call
cfg.EventRecorder.Outputs[i].Path = join(out.Path), change the comparison to use
eventrecorder.OutputFile so it matches the usage in buildOutputs and
UnmarshalYAML and keeps behavior identical while relying on the shared constant.
In `@eventrecorder/config.go`:
- Around line 82-84: The helper function name eventRecorderConfigEqual is
inconsistent with the refactored Config/Output types; rename this unexported
helper to a shorter, package-consistent name such as configEqual (or
configsEqual) and update all internal references to eventRecorderConfigEqual
accordingly so the function signature and usages match the new name while
preserving its logic for comparing Config values.
In `@eventrecorder/eventrecorder_test.go`:
- Around line 237-246: Tests still use the hardcoded "file" string for outputs;
update the two Output entries in the test fixtures (the Config/Output structs
named a and b) to use the new OutputFile constant instead of the literal "file"
so the tests align with the refactor (replace {Type: "file", ...} with {Type:
OutputFile, ...} in both places).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 23168401-53c2-4639-92a9-6bf75d4438bd
📒 Files selected for processing (6)
config/config.goeventrecorder/config.goeventrecorder/eventrecorder.goeventrecorder/eventrecorder_test.goeventrecorder/webhook.goeventrecorder/webhook_test.go
✅ Files skipped from review due to trivial changes (1)
- eventrecorder/webhook_test.go
Replace remaining "file" string literals in config/config.go and tests with the OutputFile constant, and drop the package-name stutter from eventRecorderConfigEqual -> configEqual. Signed-off-by: emreya <e.yazici1990@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
eventrecorder/eventrecorder_test.go (1)
236-236: Optional: rename test to drop the package-name stutter.Since the helper was renamed from
eventRecorderConfigEqualtoconfigEqualto avoideventrecorder.stutter, the test function name could follow suit (e.g.,TestConfigEqual) for consistency.♻️ Proposed rename
-func TestEventRecorderConfigEqual(t *testing.T) { +func TestConfigEqual(t *testing.T) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eventrecorder/eventrecorder_test.go` at line 236, Rename the test function TestEventRecorderConfigEqual to match the helper rename and drop the package-name stutter (e.g., TestConfigEqual); update any references to that test name (calls, comments) so it compiles and runs, ensuring the test still calls configEqual (the renamed helper) and keeps its signature func TestConfigEqual(t *testing.T).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@eventrecorder/eventrecorder_test.go`:
- Line 236: Rename the test function TestEventRecorderConfigEqual to match the
helper rename and drop the package-name stutter (e.g., TestConfigEqual); update
any references to that test name (calls, comments) so it compiles and runs,
ensuring the test still calls configEqual (the renamed helper) and keeps its
signature func TestConfigEqual(t *testing.T).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b0acac3e-920c-4a08-90da-0dfae1179a3a
📒 Files selected for processing (4)
config/config.goeventrecorder/config.goeventrecorder/eventrecorder.goeventrecorder/eventrecorder_test.go
✅ Files skipped from review due to trivial changes (1)
- eventrecorder/eventrecorder.go
🚧 Files skipped from review as they are similar to previous changes (1)
- config/config.go
siavashs
approved these changes
May 4, 2026
10 tasks
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
2 participants
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.
Pull Request Checklist
Please check all the applicable boxes.
benchstatto compare benchmarksWhich user-facing changes does this PR introduce?
Summary by CodeRabbit