Refactor backup and restore plugins to accept logger as a parameter and remove verbosity level configuration #58
Conversation
WalkthroughThe changes refactor the backup and restore plugin initialization to accept a logger instance as a parameter, removing internal logger creation and configuration. The Changes
Sequence Diagram(s)sequenceDiagram
participant Main
participant Logger
participant BackupPlugin
participant RestorePlugin
Main->>Logger: Obtain logger instance
Main->>BackupPlugin: NewBackupPlugin(logger)
Main->>RestorePlugin: NewRestorePlugin(logger)
BackupPlugin->>BackupPlugin: Use provided logger
RestorePlugin->>RestorePlugin: Use provided logger
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kaovilai The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
pkg/core/validation/backup.go (1)
57-59: 🛠️ Refactor suggestionRemove unused pluginVerbosityLevel handling.
The code still recognizes and logs the
pluginVerbosityLevelconfiguration key, but no longer uses it since verbosity level configuration has been removed from the plugin options. This code should be removed for clarity.- case "pluginVerbosityLevel": - p.Log.Debugf("reading/parsing pluginVerbosityLevel %s", value)
🧹 Nitpick comments (2)
pkg/core/restore.go (1)
76-84: Consider consistent field naming.The
Logfield in the validator is inconsistent with the plugin's ownlogfield. Consider standardizing the naming convention throughout the codebase for better readability.validator: &validation.RestorePluginValidator{ - Log: log, + Log: log, // Consider renaming to lowercase 'log' for consistency },pkg/core/backup.go (1)
79-79: Consider consistent field naming.Similar to the restore plugin, the
Logfield in the validator is inconsistent with the plugin's ownlogfield. Consider standardizing the naming convention throughout the codebase.validator: &validation.BackupPluginValidator{Log: log}, // Consider renaming to lowercase 'log' for consistency
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (5)
main.go(1 hunks)pkg/core/backup.go(1 hunks)pkg/core/restore.go(1 hunks)pkg/core/types/types.go(0 hunks)pkg/core/validation/backup.go(1 hunks)
💤 Files with no reviewable changes (1)
- pkg/core/types/types.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
main.go (2)
pkg/core/backup.go (1)
NewBackupPlugin(47-89)pkg/core/restore.go (1)
NewRestorePlugin(48-94)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Red Hat Konflux / hypershift-oadp-plugin-main-on-pull-request
🔇 Additional comments (6)
main.go (2)
18-18: Correct implementation of logger passing to the backup plugin.The code now properly passes the logger instance to the
NewBackupPluginfunction, aligning with the refactoring goal to have plugins accept a logger as a parameter rather than creating their own internally.
23-23: Correct implementation of logger passing to the restore plugin.The code now properly passes the logger instance to the
NewRestorePluginfunction, which is consistent with the backup plugin implementation and the overall refactoring goal.pkg/core/validation/backup.go (1)
19-19: Good interface usage for dependency inversion.Changing from a concrete logger type to the
logrus.FieldLoggerinterface improves flexibility and testability by allowing any compatible logger implementation to be injected.pkg/core/restore.go (1)
48-48: Correct implementation of logger injection.The function signature now properly accepts a
logrus.FieldLoggerparameter, enabling logger injection rather than internal creation. This is consistent with the PR objectives.pkg/core/backup.go (2)
47-47: Correct implementation of logger injection.The function signature now properly accepts a
logrus.FieldLoggerparameter, enabling logger injection rather than internal creation. This change is consistent with the PR objectives and the changes in restore.go.
73-80:✅ Verification successful
Verify all plugin options are still properly managed.
With the removal of verbosity level handling, ensure that all other plugin options (like DataUploadTimeout, DataUploadCheckPace) are still properly initialized and used throughout the code.
🏁 Script executed:
#!/bin/bash # Description: Verify all plugin options initialization and usage # Check which options are initialized in ValidatePluginConfig rg -A 3 "bo\.\w+ =" pkg/core/validation/backup.go # Check which options are used in backup.go rg "p\.(DataUploadTimeout|DataUploadCheckPace)" pkg/core/backup.goLength of output: 1167
Plugin options are properly managed
DataUploadTimeout and DataUploadCheckPace are both initialized inpkg/core/validation/backup.goand consumed inpkg/core/backup.go(passed tocommon.WaitForDataUploadandcommon.WaitForPodVolumeBackup). No further action required.
|
hypershift-oadp-plugin/pkg/core/backup.go Line 132 in f37e2c8 https://termbin.com/lswl This is suspect: |
|
@jparrill can you please poke at this pr seeing some issues here. |
It's right here boss. |
|
For extra clarity |
…nd remove verbosity level configuration Signed-off-by: Tiger Kaovilai <[email protected]>
kaovilai
force-pushed
the
use-velero-plugin-log
branch
from
d6bbd7d to
2b3f460
Compare
|
@kaovilai: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Signed-off-by: Tiger Kaovilai [email protected]
What this PR does / why we need it
So all log operations are routed to velero server and to not cause issues with go-plugin.
hypershift-oadp-plugin/vendor/github.com/vmware-tanzu/velero/pkg/plugin/framework/logger.go
Lines 25 to 35 in 2b3f460
Which issue(s) this PR fixes
Fixes #
Checklist
Summary by CodeRabbit
Refactor
Chores