Repository navigation
Feat/backup work consolidation - #1968
Conversation
Signed-off-by: Andrey Kolkov <[email protected]>
Signed-off-by: Andrey Kolkov <[email protected]>
Signed-off-by: Andrey Kolkov <[email protected]>
Signed-off-by: Timofei Larkin <[email protected]>
<!-- Thank you for making a contribution! Here are some tips for you: - Start the PR title with the [label] of Cozystack component: - For system components: [platform], [system], [linstor], [cilium], [kube-ovn], [dashboard], [cluster-api], etc. - For managed apps: [apps], [tenant], [kubernetes], [postgres], [virtual-machine] etc. - For development and maintenance: [tests], [ci], [docs], [maintenance]. - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does ### Release note <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same [label] as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note Implement restoreJob controller and velero strategy. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added a RestoreJob controller with manager registration to handle restore operations. * Added Velero-based and Job-based restore workflows with lifecycle handling, requeueing, and polling. * Velero templates now include a RestoreSpec field to configure restores. * **Bug Fixes / Reliability** * New helpers to mark BackupJob and RestoreJob failures and reliably update status. * **Documentation / CRD** * RestoreJob CRD gains a status subresource and a Phase print column. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
<!-- Thank you for making a contribution! Here are some tips for you: - Start the PR title with the [label] of Cozystack component: - For system components: [platform], [system], [linstor], [cilium], [kube-ovn], [dashboard], [cluster-api], etc. - For managed apps: [apps], [tenant], [kubernetes], [postgres], [virtual-machine] etc. - For development and maintenance: [tests], [ci], [docs], [maintenance]. - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does ### Release note <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same [label] as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note [ - add BackupClass Velero and strategy for VM - dashboard fixes for BackupClass and simplifed Plan/BackupJob API ] ```
dosubot
Bot
added
size/L
📝 WalkthroughWalkthroughIntroduces a RestoreJob reconciler and Velero restore templating/polling, adds a RestoreSpec to Velero strategy types/CRD, removes BackupJob webhook and tests, tightens BackupJob validation and CRDs, updates dashboard form overrides and templates, and adjusts controller wiring and codegen script. Changes
Sequence DiagramsequenceDiagram
participant RJ as RestoreJob (resource)
participant R as RestoreJobReconciler (controller)
participant B as Backup (resource)
participant S as Strategy (Velero resource)
participant V as Velero Restore (resource)
participant API as Velero API (polling)
RJ->>R: Reconcile request
R->>R: Skip if completed?
R->>B: Fetch Backup referenced by RestoreJob
B-->>R: Return Backup
R->>S: Resolve strategy from Backup.Spec.StrategyRef
S-->>R: Return strategy (may include RestoreSpec)
alt Velero strategy
R->>R: Template RestoreSpec using strategy.template
R->>V: Create or fetch Velero Restore in velero namespace
V-->>R: Return Restore object
R->>API: Poll restore status
API-->>R: Return running/succeeded/failed
R->>RJ: Update RestoreJob status (phase, conditions)
else Job strategy
R->>R: Call reconcileJobRestore (job-specific flow)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 Timofei Larkin (@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 consolidates backup and restore functionalities by introducing a Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
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 (@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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces restore functionality by adding the RestoreJob resource and its corresponding controller logic, focusing on the Velero backup strategy. The changes are well-structured, consolidating strategy-specific logic into the backupstrategy-controller. However, I've identified a few critical issues: a design flaw in the restore logic that prevents the use of parameters from the BackupClass, an incorrect Velero backup template that would cause backup creation to fail, and a potential panic in the restore controller due to a nil pointer dereference. I've also included a suggestion to improve error logging.
| if restoreJob.Spec.TargetApplicationRef != nil { | ||
| return *restoreJob.Spec.TargetApplicationRef | ||
| } |
There was a problem hiding this comment.
The getTargetApplicationRef function returns *restoreJob.Spec.TargetApplicationRef directly if it's not nil. However, TargetApplicationRef is a *corev1.TypedLocalObjectReference, and its APIGroup field can be nil if not specified by the user.
Later in createVeleroRestore, *targetAppRef.APIGroup is dereferenced, which will cause a panic if APIGroup is nil.
You should normalize the TargetApplicationRef to ensure APIGroup has a default value, similar to how BackupJob.Spec.ApplicationRef is handled.
| if restoreJob.Spec.TargetApplicationRef != nil { | |
| return *restoreJob.Spec.TargetApplicationRef | |
| } | |
| if restoreJob.Spec.TargetApplicationRef != nil { | |
| return backupsv1alpha1.NormalizeApplicationRef(*restoreJob.Spec.TargetApplicationRef) | |
| } |
| if err := r.Create(ctx, veleroRestore); err != nil { | ||
| if veleroRestore.Name != "" { | ||
| name = veleroRestore.Name | ||
| } | ||
| logger.Error(err, "failed to create Velero Restore", "name", veleroRestore.Name) | ||
| r.Recorder.Event(restoreJob, corev1.EventTypeWarning, "VeleroRestoreCreationFailed", | ||
| fmt.Sprintf("Failed to create Velero Restore %s/%s: %v", veleroNamespace, name, err)) | ||
| return err |
There was a problem hiding this comment.
The error handling logic here can be simplified. When r.Create fails, veleroRestore.Name will be empty because only GenerateName was set. The log message logger.Error(..., "name", veleroRestore.Name) will therefore log an empty name. The if veleroRestore.Name != "" check is also unlikely to be true on creation failure.
It would be clearer to log GenerateName and remove the redundant check.
if err := r.Create(ctx, veleroRestore); err != nil {
logger.Error(err, "failed to create Velero Restore", "generateName", name)
r.Recorder.Event(restoreJob, corev1.EventTypeWarning, "VeleroRestoreCreationFailed",
fmt.Sprintf("Failed to create Velero Restore %s/%s: %v", veleroNamespace, name, err))
return err
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@api/backups/strategy/v1alpha1/velero_types.go`:
- Around line 58-59: The RestoreSpec field is currently required in the CRD;
make it optional to preserve backward compatibility by changing the RestoreSpec
field to a pointer to velerov1.RestoreSpec, add the json tag with omitempty
(e.g., `json:"restoreSpec,omitempty"`), and annotate it with the kubebuilder
+optional comment so the CRD generator marks it optional; update any usage of
RestoreSpec in methods (e.g., validation or defaulting on the type that contains
RestoreSpec) to handle a nil pointer.
In `@packages/system/backup-controller/templates/class.yaml`:
- Around line 1-14: The manifest for the BackupClass resource is missing
apiVersion which makes it invalid; add an appropriate apiVersion for the
BackupClass resource at the top of the YAML (for example apiVersion:
backups.cozystack.io/v1) so the resource kind BackupClass with metadata.name
velero and the spec (strategyRef, application, parameters) is valid in
Kubernetes.
In `@packages/system/virtual-machine-rd/cozyrds/backup-strategy.yaml`:
- Around line 1-32: The Velero strategy YAML uses spec.backupTemplate which
doesn't match VeleroSpec's expected json field name; rename the top-level
spec.backupTemplate key to spec.template (preserving the nested spec: ... block
and all child fields like includedNamespaces, labelSelector, includedResources,
storageLocation, volumeSnapshotLocations, snapshotVolumes, snapshotMoveData,
ttl, itemOperationTimeout) so the Velero resource (kind: Velero) is decoded
correctly at runtime; search for the Velero resource named
velero-backup-strategy-for-virtualmachines and replace backupTemplate → template
accordingly.
🧹 Nitpick comments (3)
internal/backupcontroller/backupjob_controller.go (1)
121-143: Prefermeta.SetStatusConditionto avoid duplicate Ready conditions.
Repeated failure reconciles will keep appending, causing unbounded growth and stale condition ordering.♻️ Suggested refactor
- backupJob.Status.Conditions = append(backupJob.Status.Conditions, metav1.Condition{ + meta.SetStatusCondition(&backupJob.Status.Conditions, metav1.Condition{ Type: "Ready", Status: metav1.ConditionFalse, Reason: "BackupFailed", Message: message, LastTransitionTime: now, })internal/backupcontroller/velerostrategy_controller.go (1)
478-483: Template context inconsistency between backup and restore.In
createVeleroBackup(Lines 252-256), the template context is structured as:templateContext := map[string]interface{}{ "Application": app.Object, "Parameters": resolved.Parameters, }However, here in
createVeleroRestore, the template receivesapp.Objectdirectly. This means:
- Backup templates: use
{{ .Application.metadata.name }}- Restore templates: use
{{ .metadata.name }}This inconsistency could cause confusion for users writing templates. Consider aligning the template context structure.
♻️ Proposed fix to align template contexts
- // Template the restore spec from the strategy - veleroRestoreSpec, err := template.Template(&strategy.Spec.Template.RestoreSpec, app.Object) + // Template the restore spec from the strategy + templateContext := map[string]interface{}{ + "Application": app.Object, + } + veleroRestoreSpec, err := template.Template(&strategy.Spec.Template.RestoreSpec, templateContext)internal/backupcontroller/restorejob_controller.go (1)
124-131: Usemeta.SetStatusConditionto avoid duplicate conditions.Using
append()directly can lead to duplicate "Ready" conditions ifmarkRestoreJobFailedis called multiple times during retries or error recovery. Themeta.SetStatusConditionutility fromk8s.io/apimachinery/pkg/api/metahandles deduplication and proper transition time updates.The same issue exists in
markBackupJobFailedatvelerostrategy_controller.go:219.♻️ Proposed fix using meta.SetStatusCondition
// Add condition - restoreJob.Status.Conditions = append(restoreJob.Status.Conditions, metav1.Condition{ + meta.SetStatusCondition(&restoreJob.Status.Conditions, metav1.Condition{ Type: "Ready", Status: metav1.ConditionFalse, Reason: "RestoreFailed", Message: message, LastTransitionTime: now, })Note: The import
"k8s.io/apimachinery/pkg/api/meta"is already present (Line 10).
dosubot
Bot
added
size/XXL
766d8e4 to
6329214
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/backupcontroller/restorejob_controller.go`:
- Around line 106-115: The fallback path in getTargetApplicationRef returns
backup.Spec.ApplicationRef without normalization, which can leave APIGroup nil
and cause panics; fix it by passing the fallback reference through
backupsv1alpha1.NormalizeApplicationRef before returning (i.e., when
restoreJob.Spec.TargetApplicationRef is nil, call
NormalizeApplicationRef(backup.Spec.ApplicationRef) and return the result) so
the returned TypedLocalObjectReference is always normalized.
🧹 Nitpick comments (3)
internal/backupcontroller/backupjob_controller.go (1)
128-134: Usemeta.SetStatusConditionfor proper condition management.Appending conditions directly can lead to duplicate "Ready" conditions if the method is called multiple times. Use
meta.SetStatusConditionfromk8s.io/apimachinery/pkg/api/meta(already imported) to ensure proper upsert behavior.♻️ Proposed fix
- // Add condition - backupJob.Status.Conditions = append(backupJob.Status.Conditions, metav1.Condition{ + // Set condition (upserts existing condition of same type) + meta.SetStatusCondition(&backupJob.Status.Conditions, metav1.Condition{ Type: "Ready", Status: metav1.ConditionFalse, Reason: "BackupFailed", Message: message, LastTransitionTime: now, })internal/backupcontroller/velerostrategy_controller.go (1)
455-461: TODO: Persist Parameters for restore templating.Right now templates always see empty Parameters, which can silently break template expectations. Consider persisting Parameters on the Backup or RestoreJob and wiring them into the context.
If you want, I can draft the persistence + wiring patch or open a tracking issue.
internal/backupcontroller/restorejob_controller.go (1)
117-139: Prefermeta.SetStatusConditionto avoid duplicate Ready conditions.Appending can accumulate duplicates on retry;
SetStatusConditionupserts and preserves transition semantics.♻️ Suggested refactor
- // Add condition - restoreJob.Status.Conditions = append(restoreJob.Status.Conditions, metav1.Condition{ + // Add/replace condition + meta.SetStatusCondition(&restoreJob.Status.Conditions, metav1.Condition{ Type: "Ready", Status: metav1.ConditionFalse, Reason: "RestoreFailed", Message: message, LastTransitionTime: now, })
| // getTargetApplicationRef determines the effective target application reference. | ||
| // According to DESIGN.md, if spec.targetApplicationRef is omitted, drivers SHOULD | ||
| // restore into backup.spec.applicationRef. | ||
| // The returned reference is normalized to ensure APIGroup has a default value. | ||
| func (r *RestoreJobReconciler) getTargetApplicationRef(restoreJob *backupsv1alpha1.RestoreJob, backup *backupsv1alpha1.Backup) corev1.TypedLocalObjectReference { | ||
| if restoreJob.Spec.TargetApplicationRef != nil { | ||
| return backupsv1alpha1.NormalizeApplicationRef(*restoreJob.Spec.TargetApplicationRef) | ||
| } | ||
| return backup.Spec.ApplicationRef | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and examine the NormalizeApplicationRef function
rg "NormalizeApplicationRef" --type go -A 10 -B 2Repository: cozystack/cozystack
Length of output: 9620
🏁 Script executed:
# Check the types involved - TypedLocalObjectReference and ApplicationRef
rg "type ApplicationRef" --type go -A 5
rg "TypedLocalObjectReference" --type go -B 2 -A 5Repository: cozystack/cozystack
Length of output: 29725
🏁 Script executed:
# Find usages of getTargetApplicationRef to see downstream code
rg "getTargetApplicationRef" --type go -B 2 -A 5Repository: cozystack/cozystack
Length of output: 2114
🏁 Script executed:
# Check the actual file to understand context around the function
cat -n internal/backupcontroller/restorejob_controller.go | sed -n '100,120p'Repository: cozystack/cozystack
Length of output: 1226
Normalize the fallback application ref to ensure consistent APIGroup handling.
The function documentation promises normalization of the returned reference, but the fallback path bypasses this. If backup.Spec.ApplicationRef.APIGroup is nil (common for core group resources), downstream code that dereferences APIGroup without null checks will panic.
Proposed fix
func (r *RestoreJobReconciler) getTargetApplicationRef(restoreJob *backupsv1alpha1.RestoreJob, backup *backupsv1alpha1.Backup) corev1.TypedLocalObjectReference {
if restoreJob.Spec.TargetApplicationRef != nil {
return backupsv1alpha1.NormalizeApplicationRef(*restoreJob.Spec.TargetApplicationRef)
}
- return backup.Spec.ApplicationRef
+ return backupsv1alpha1.NormalizeApplicationRef(backup.Spec.ApplicationRef)
}🤖 Prompt for AI Agents
In `@internal/backupcontroller/restorejob_controller.go` around lines 106 - 115,
The fallback path in getTargetApplicationRef returns backup.Spec.ApplicationRef
without normalization, which can leave APIGroup nil and cause panics; fix it by
passing the fallback reference through backupsv1alpha1.NormalizeApplicationRef
before returning (i.e., when restoreJob.Spec.TargetApplicationRef is nil, call
NormalizeApplicationRef(backup.Spec.ApplicationRef) and return the result) so
the returned TypedLocalObjectReference is always normalized.
Signed-off-by: Timofei Larkin <[email protected]>
6329214 to
000b5ff
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@hack/update-codegen.sh`:
- Around line 21-23: The CODEGEN_PKG default should not use a literal tilde;
change the default to resolve to the Go module cache dynamically: replace the
${CODEGEN_PKG:-~/go/pkg/mod/k8s.io/[email protected]} pattern with logic
that uses ${CODEGEN_PKG:-$(go env GOMODCACHE)/k8s.io/[email protected]} and
fall back to $(go env GOPATH)/pkg/mod if GOMODCACHE is empty, ensuring
SCRIPT_ROOT, CODEGEN_PKG and API_KNOWN_VIOLATIONS_DIR remain unchanged
otherwise; implement this by computing a resolved_mod_cache variable via go env
and then setting CODEGEN_PKG to
${CODEGEN_PKG:-"$resolved_mod_cache/k8s.io/[email protected]"}.
🧹 Nitpick comments (3)
internal/backupcontroller/backupjob_controller.go (1)
128-134: Consider usingmeta.SetStatusConditionto upsert conditions.Appending directly to
Conditionsmay create duplicates ifmarkBackupJobFailedis called multiple times for the same BackupJob. Theapimachinery/pkg/api/metapackage providesSetStatusConditionwhich handles upsert semantics.♻️ Proposed fix
+ meta.SetStatusCondition(&backupJob.Status.Conditions, metav1.Condition{ - backupJob.Status.Conditions = append(backupJob.Status.Conditions, metav1.Condition{ Type: "Ready", Status: metav1.ConditionFalse, Reason: "BackupFailed", Message: message, LastTransitionTime: now, })internal/backupcontroller/velerostrategy_controller.go (1)
458-460: TODO: persist Parameters for restore templating.Line 458 calls out the limitation; I can help sketch a follow-up to persist Parameters on Backup and wire them into the restore context.
internal/backupcontroller/restorejob_controller.go (1)
117-133: Usemeta.SetStatusConditionto prevent duplicate Ready conditions.Appending conditions on line 126 creates duplicates on every failed reconcile. The
meta.SetStatusConditionfunction upserts by condition type, preventing bloat and avoiding noisy UI updates. Themetapackage is already imported.♻️ Suggested refactor
- restoreJob.Status.Conditions = append(restoreJob.Status.Conditions, metav1.Condition{ + meta.SetStatusCondition(&restoreJob.Status.Conditions, metav1.Condition{ Type: "Ready", Status: metav1.ConditionFalse, Reason: "RestoreFailed", Message: message, LastTransitionTime: now, })
| SCRIPT_ROOT=$(dirname "${BASH_SOURCE[0]}")/.. | ||
| CODEGEN_PKG=${CODEGEN_PKG:-$(cd "${SCRIPT_ROOT}"; ls -d -1 ./vendor/k8s.io/code-generator 2>/dev/null || echo ../code-generator)} | ||
| CODEGEN_PKG=${CODEGEN_PKG:-~/go/pkg/mod/k8s.io/code-generator@v0.34.1} | ||
| API_KNOWN_VIOLATIONS_DIR="${API_KNOWN_VIOLATIONS_DIR:-"${SCRIPT_ROOT}/api/api-rules"}" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, read the file to see the actual context
cat -n hack/update-codegen.sh | head -30Repository: cozystack/cozystack
Length of output: 1557
🏁 Script executed:
# Verify tilde expansion behavior in parameter expansion with bash
bash << 'EOF'
# Test 1: Does tilde expand in parameter expansion?
test_var="${HOME_LITERAL:-~/test}"
echo "Test 1 - Direct parameter expansion: $test_var"
# Test 2: What about with variable?
HOME_TEST=~/another
test_var2="${UNDEFINED:-$HOME_TEST}"
echo "Test 2 - Variable parameter expansion: $test_var2"
# Test 3: Quoted tilde
test_var3="${UNDEFINED:-~/direct}"
echo "Test 3 - Quoted tilde: $test_var3"
# Test 4: Does the literal path work if file exists?
echo "Test 4 - Checking if literal ~/go exists:"
[[ -d ~/go ]] && echo " ~/go exists" || echo " ~/go does not exist"
EOFRepository: cozystack/cozystack
Length of output: 264
🏁 Script executed:
# Check if GOMODCACHE is set and what the typical path looks like
bash << 'EOF'
echo "GOMODCACHE: $(go env GOMODCACHE)"
echo "GOPATH: $(go env GOPATH)"
echo "GOROOT: $(go env GOROOT)"
# Check if k8s.io/code-generator is available
echo "Checking k8s.io/code-generator:"
go list -m -f '{{.Dir}}' k8s.io/[email protected] 2>/dev/null || echo "Module not found in go.mod"
EOFRepository: cozystack/cozystack
Length of output: 180
🏁 Script executed:
# Check how the script file uses CODEGEN_PKG
rg -A 5 "CODEGEN_PKG" hack/update-codegen.shRepository: cozystack/cozystack
Length of output: 609
🏁 Script executed:
# Check how this script is invoked - search for references to it
rg "update-codegen" --type sh --type yml --type yaml --type makefile -B 2 -A 2Repository: cozystack/cozystack
Length of output: 90
🏁 Script executed:
# Check if there's a Makefile that runs this
fd -e Makefile -e makefile | head -5 | xargs grep -l "codegen\|update-codegen" 2>/dev/null || echo "No Makefile references found"Repository: cozystack/cozystack
Length of output: 91
🏁 Script executed:
# Check CI configuration
find . -maxdepth 2 -name "*.yml" -o -name "*.yaml" | head -10 | xargs grep -l "codegen\|update-codegen" 2>/dev/null || echo "No CI references found"Repository: cozystack/cozystack
Length of output: 85
🏁 Script executed:
# Verify the fix: can we actually use go env GOMODCACHE?
bash << 'EOF'
GOMODCACHE=${GOMODCACHE:-"$(go env GOMODCACHE)"}
echo "GOMODCACHE resolved to: $GOMODCACHE"
CODEGEN_PKG="${GOMODCACHE}/k8s.io/[email protected]"
echo "CODEGEN_PKG would be: $CODEGEN_PKG"
echo "Checking if directory path makes sense: $(ls -d ${GOMODCACHE}/k8s.io 2>/dev/null && echo 'k8s.io dir exists' || echo 'k8s.io dir does not exist')"
EOFRepository: cozystack/cozystack
Length of output: 256
🏁 Script executed:
# Check if k8s.io/code-generator is in go.mod
rg "k8s.io/code-generator" go.modRepository: cozystack/cozystack
Length of output: 45
🏁 Script executed:
# Check README and docs for references
find . -maxdepth 2 \( -name "README*" -o -name "*.md" \) | xargs grep -l "codegen\|update-codegen" 2>/dev/null || echo "No docs reference found"Repository: cozystack/cozystack
Length of output: 86
🏁 Script executed:
# Check if the script is meant to be run and what it does
head -50 hack/update-codegen.sh | tail -30Repository: cozystack/cozystack
Length of output: 1357
🏁 Script executed:
# Look for any shell scripts that might call this
fd -e sh | xargs grep -l "update-codegen" 2>/dev/null | head -5Repository: cozystack/cozystack
Length of output: 45
Fix tilde expansion in CODEGEN_PKG default path.
The literal ~ in ${CODEGEN_PKG:-~/go/pkg/mod/...} does not expand within parameter expansion, so the default becomes a literal path that will fail when passed to source. Also, hardcoding ~/go ignores custom GOMODCACHE and breaks on clean environments.
Note: This script is not currently referenced in CI or Makefiles, but should still be fixed for portability if it's meant to be available for manual use.
Suggested fix
SCRIPT_ROOT=$(dirname "${BASH_SOURCE[0]}")/..
-CODEGEN_PKG=${CODEGEN_PKG:-~/go/pkg/mod/k8s.io/[email protected]}
+GOMODCACHE=${GOMODCACHE:-"$(go env GOMODCACHE)"}
+CODEGEN_PKG=${CODEGEN_PKG:-"${GOMODCACHE}/k8s.io/[email protected]"}
API_KNOWN_VIOLATIONS_DIR="${API_KNOWN_VIOLATIONS_DIR:-"${SCRIPT_ROOT}/api/api-rules"}"🤖 Prompt for AI Agents
In `@hack/update-codegen.sh` around lines 21 - 23, The CODEGEN_PKG default should
not use a literal tilde; change the default to resolve to the Go module cache
dynamically: replace the
${CODEGEN_PKG:-~/go/pkg/mod/k8s.io/[email protected]} pattern with logic
that uses ${CODEGEN_PKG:-$(go env GOMODCACHE)/k8s.io/[email protected]} and
fall back to $(go env GOPATH)/pkg/mod if GOMODCACHE is empty, ensuring
SCRIPT_ROOT, CODEGEN_PKG and API_KNOWN_VIOLATIONS_DIR remain unchanged
otherwise; implement this by computing a resolved_mod_cache variable via go env
and then setting CODEGEN_PKG to
${CODEGEN_PKG:-"$resolved_mod_cache/k8s.io/[email protected]"}.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/system/virtual-machine-rd/backupstrategy/backup-strategy.yaml`:
- Around line 11-12: The labelSelector in the BackupSpec is malformed: move the
flat key(s) under a metav1.LabelSelector matchLabels map. Update the
labelSelector block (symbol: labelSelector) to use matchLabels and place
apps.cozystack.io/application.Kind: '{{ .Application.kind }}' inside that map so
the BackupSpec validation accepts it (reference BackupSpec /
metav1.LabelSelector).
cf7bbb7 to
9fac9c9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/system/virtual-machine-rd/templates/backupstrategy.yaml`:
- Around line 12-13: The labelSelector is missing the required matchLabels
wrapper and will fail CRD validation; update the labelSelector block in
templates/backupstrategy.yaml so that labelSelector contains a matchLabels map
and move the existing key "apps.cozystack.io/application.Kind: '{{ printf \"{{
.Application.kind }}\" }}'" under matchLabels. Keep the same key and templating
expression but nest it under matchLabels to conform to metav1.LabelSelector used
by BackupSpec.
🧹 Nitpick comments (1)
packages/system/virtual-machine-rd/templates/backupstrategy.yaml (1)
8-8: Outdated doc reference in comment.The comment links to Velero v1.9 docs. Consider updating to the current version for accuracy.
Signed-off-by: Timofei Larkin <[email protected]>
9fac9c9 to
ffd97e5
Compare
Signed-off-by: Timofei Larkin <[email protected]>
3 participants
Summary by CodeRabbit
New Features
Bug Fixes
Chores