types/setting.go (4)

34-35: Add doc comment for newly introduced constant
It's helpful to document that 300 seconds is the default interval for polling the backup store to make it more intuitive for future maintainers.

---

1559-1559: Consider adding a docstring
It's a good practice to describe the purpose and expected behavior of GetCustomizedDefaultSettings in a docstring, especially given its central role in customizing settings.

---

1562-1563: Initialize the map in a single line
Combining line 1562 and 1563 for map initialization can reduce verbosity, e.g., defaultBackupTargetSettings := map[string]string{}.

---

1571-1577: Improve readability for setting checks
Rather than chaining multiple || expressions, consider a short set membership check. This becomes more maintainable if new backup-target-related settings are introduced later.

datastore/longhorn.go (2)

9-9: Initialize a random seed
If randomness is used in scheduling or selection logic, consider calling rand.Seed(time.Now().UnixNano()) once at startup to avoid deterministic outcomes.

---

78-81: Add error context logging
Including more detailed logging (e.g., error message or relevant fields) upon failure of syncDefaultBackupTargetResourceWithCustomizedSettings can help diagnose issues.

webhook/resources/backuptarget/validator.go (1)

156-166: Refine whitespace checks
This switch statement thoroughly guards against leading/trailing spaces or newlines. If multiple whitespace issues exist in one key, consider combining them into a single, more descriptive error message for clarity.

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

datastore/longhorn.go (1)

218-279: Enhance error handling in syncDefaultBackupTargetResourceWithCustomizedSettings

The function handles backup target synchronization well, but could benefit from some improvements:

1. Consider adding validation for pollInterval value range
2. Consider adding logging for important state transitions

 func (s *DataStore) syncDefaultBackupTargetResourceWithCustomizedSettings(customizedDefaultBackupTargetSettings map[string]string) error {
+    logrus.Debugf("Syncing default backup target with settings: %v", customizedDefaultBackupTargetSettings)
+
     backupTarget := &longhorn.BackupTarget{
         ObjectMeta: metav1.ObjectMeta{
             Name: types.DefaultBackupTargetName,
@@ -238,6 +240,9 @@
     if pollIntervalStr != "" {
         pollIntervalAsInt, err := strconv.ParseInt(pollIntervalStr, 10, 64)
         if err != nil {
+            return errors.Wrapf(err, "invalid poll interval value %v", pollIntervalStr)
+        }
+        if pollIntervalAsInt < 0 {
             return err
         }
         backupTarget.Spec.PollInterval = metav1.Duration{Duration: time.Duration(pollIntervalAsInt) * time.Second}

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

#!/bin/bash
# Search for all callers of GetCustomizedDefaultSettings to verify they handle the new return value
rg "GetCustomizedDefaultSettings\(" -A 3

#!/bin/bash
# Check how customizedDefaultBackupTargetSettings is used in datastore/longhorn.go
rg "customizedDefaultBackupTargetSettings" -A 10 datastore/longhorn.go

types/setting.go (1)

Line range hint 1559-1627: Consider adding error handling for invalid poll interval string

The function correctly separates backup target settings but should validate the poll interval string format before returning it.

Consider adding validation like:

 if pollIntervalStr != "" {
+    if _, err := strconv.ParseInt(pollIntervalStr, 10, 64); err != nil {
+        return nil, nil, fmt.Errorf("invalid poll interval format %v: %v", pollIntervalStr, err)
+    }
 }

datastore/longhorn.go (2)

238-244: Add validation for poll interval range

The code should validate that the poll interval is within a reasonable range.

Consider adding range validation:

 if pollIntervalStr != "" {
     pollIntervalAsInt, err := strconv.ParseInt(pollIntervalStr, 10, 64)
     if err != nil {
         return err
     }
+    if pollIntervalAsInt < 0 {
+        return fmt.Errorf("poll interval %v must be non-negative", pollIntervalAsInt)
+    }
     backupTarget.Spec.PollInterval = metav1.Duration{Duration: time.Duration(pollIntervalAsInt) * time.Second}
 }

---

272-279: Consider adding logging for backup target updates

Adding debug logs would help track backup target configuration changes.

Consider adding:

 if reflect.DeepEqual(existingBackupTarget.Spec, backupTarget.Spec) {
+    logrus.Debugf("No changes detected for backup target %v", backupTarget.Name)
     return nil
 }
+logrus.Infof("Updating backup target %v", backupTarget.Name)

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

datastore/longhorn.go (1)

218-279: Add validation for poll interval value range

The function handles backup target synchronization well, but should validate that the poll interval value is within a reasonable range to prevent potential issues with very short or very long intervals.

 if pollIntervalStr != "" {
     pollIntervalAsInt, err := strconv.ParseInt(pollIntervalStr, 10, 64)
     if err != nil {
         return err
     }
+    // Add validation for reasonable poll interval range (e.g., 30s to 24h)
+    if pollIntervalAsInt < 30 || pollIntervalAsInt > 86400 {
+        return fmt.Errorf("poll interval must be between 30 and 86400 seconds")
+    }
     backupTarget.Spec.PollInterval = metav1.Duration{Duration: time.Duration(pollIntervalAsInt) * time.Second}
 }

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro