[backups][WIP] Draft Job strategy implementation#1721
[backups][WIP] Draft Job strategy implementation#1721lllamnyp wants to merge 1 commit intomaincozystack/cozystack:mainfrom feat/jobdriver-implementationcozystack/cozystack:feat/jobdriver-implementationCopy head branch name to clipboard
Conversation
Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ 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 |
Summary of ChangesHello @lllamnyp, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request lays the foundational groundwork for a robust backup system by implementing the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new JobStrategy implementation for BackupJob resources, allowing the creation of Kubernetes Jobs based on templated PodTemplateSpec definitions. It also includes a generic templating utility to process Go template expressions within Kubernetes objects. The changes are a good step towards a flexible backup strategy. However, there are a few areas that could be improved for robustness and error handling, particularly regarding the templating logic and handling of API group references.
| templateFunc := func(in string) string { | ||
| out, err := template(in, templateContext) | ||
| if err != nil { | ||
| return in | ||
| } | ||
| return out | ||
| } |
There was a problem hiding this comment.
The templateFunc silently swallows errors from template(in, templateContext), returning the original string in instead. This can lead to subtle bugs where template expressions are malformed or refer to non-existent fields, but no error is propagated, and the resulting object contains untemplated strings. It would be more robust to propagate these errors or at least log them with a higher severity to make debugging easier.
| templateFunc := func(in string) string { | |
| out, err := template(in, templateContext) | |
| if err != nil { | |
| return in | |
| } | |
| return out | |
| } | |
| templateFunc := func(in string) string { | |
| out, err := template(in, templateContext) | |
| if err != nil { | |
| // TODO: Log this error or return it from Template function for better error visibility | |
| return in | |
| } | |
| return out | |
| } |
| if j.Spec.ApplicationRef.APIGroup != nil { | ||
| applicationRefAPIGroup = *j.Spec.ApplicationRef.APIGroup | ||
| } | ||
| if j.Spec.StrategyRef.APIGroup != nil { | ||
| strategyRefAPIGroup = *j.Spec.StrategyRef.APIGroup | ||
| } | ||
| if j.Spec.StorageRef.APIGroup != nil { | ||
| storageRefAPIGroup = *j.Spec.StorageRef.APIGroup | ||
| } |
There was a problem hiding this comment.
The APIGroup fields in TypedLocalObjectReference are pointers to strings. If they are nil, the current logic initializes applicationRefAPIGroup, strategyRefAPIGroup, and storageRefAPIGroup to empty strings. While an empty string for APIGroup typically refers to the core Kubernetes API group, this behavior is implicit and not explicitly handled or documented. It might be clearer to explicitly check for nil and assign "" if that's the intended behavior for core API group resources, or handle it as an error if APIGroup is always expected to be set for non-core resources.
var applicationRefAPIGroup string
var strategyRefAPIGroup string
var storageRefAPIGroup string
if j.Spec.ApplicationRef.APIGroup != nil {
applicationRefAPIGroup = *j.Spec.ApplicationRef.APIGroup
} else {
// Default to core API group if not specified
applicationRefAPIGroup = ""
}
if j.Spec.StrategyRef.APIGroup != nil {
strategyRefAPIGroup = *j.Spec.StrategyRef.APIGroup
} else {
strategyRefAPIGroup = ""
}
if j.Spec.StorageRef.APIGroup != nil {
storageRefAPIGroup = *j.Spec.StorageRef.APIGroup
} else {
storageRefAPIGroup = ""
}| values, ok := app.Object["spec"].(map[string]any) | ||
| if !ok { | ||
| values = map[string]any{} | ||
| } |
There was a problem hiding this comment.
The type assertion app.Object["spec"].(map[string]any) and subsequent fallback to an empty map if the assertion fails could mask potential issues. If the application object's spec is expected to always be a map[string]any, a failure here might indicate a malformed resource or an unexpected API version. It would be safer to log a warning or return an error in such cases, rather than silently proceeding with an empty map, which could lead to incorrect template rendering.
values, ok := app.Object["spec"].(map[string]any)
if !ok {
log.Info("Application object 'spec' field is not a map[string]any, using empty values", "application", client.ObjectKeyFromObject(app))
values = map[string]any{}
}
TODO