refactor(e2e): use helm install instead of kubectl apply for cozystack installation#2060
refactor(e2e): use helm install instead of kubectl apply for cozystack installation#2060
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds embedded CRD manifests and a crdinstall package to server-side apply CRDs at operator startup; switches e2e install to Helm-based Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Summary of ChangesHello @lexfrei, 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 refactors the end-to-end testing setup for Cozystack by transitioning from a 'kubectl apply' based installation of static YAML manifests to a more robust and streamlined Helm-driven deployment. This change simplifies the E2E test environment, reduces CI overhead by removing unnecessary artifact handling, and aligns the testing process with Helm's capabilities for managing Kubernetes applications. 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
Ignored Files
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
|
![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 refactors the E2E tests to use helm upgrade --install for deploying Cozystack, replacing the previous method of applying static YAMLs with kubectl apply. This is a great improvement as it aligns the testing process more closely with the recommended installation procedure and simplifies the test setup. The related cleanup in the Makefile is also correct. However, I've found a critical syntax error in the new test script that will cause it to fail, along with a minor redundancy. Please see my comment for details.
lexfrei
requested review from
IvanHunters,
androndo,
kvaps and
lllamnyp
as code owners
dosubot
bot
added
the
size:M
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@hack/e2e-install-cozystack.bats`:
- Around line 11-15: The pipeline running `helm template ... | kubectl apply
...` can hide a failing `helm template` because pipefail is not enabled; update
the script to ensure `helm template` failures are detected (e.g., enable `set -o
pipefail` / `set -euo pipefail` at the top of the bats script or explicitly
capture and check the exit status of the `helm template` command before piping
to `kubectl apply`) so that errors from the `helm template` step abort the test
instead of being swallowed by the pipe.
hack/e2e-install-cozystack.bats
Outdated
| # Install CRDs first (PackageSource CR in the chart depends on them) | ||
| helm template installer packages/core/installer \ | ||
| --namespace cozy-system \ | ||
| --show-only templates/crds.yaml \ | ||
| | kubectl apply --server-side -f - |
There was a problem hiding this comment.
Pipe may silently swallow a helm template failure.
Bats does not enable pipefail by default. If helm template fails (e.g., bad values, missing template), the pipeline's exit code is determined solely by kubectl apply, which can succeed on empty stdin. This would mask the error and let the test proceed with no CRDs applied.
Proposed fix
`@test` "Install Cozystack" {
# Install CRDs first (PackageSource CR in the chart depends on them)
+ run bash -o pipefail -c '
helm template installer packages/core/installer \
--namespace cozy-system \
--show-only templates/crds.yaml \
- | kubectl apply --server-side -f -
+ | kubectl apply --server-side -f -'
+ [ "$status" -eq 0 ]Alternatively, a simpler approach that stays closer to the current style:
# Install CRDs first (PackageSource CR in the chart depends on them)
+ helm template installer packages/core/installer \
+ --namespace cozy-system \
+ --show-only templates/crds.yaml \
+ > /tmp/crds.yaml
+ kubectl apply --server-side -f /tmp/crds.yaml
- helm template installer packages/core/installer \
- --namespace cozy-system \
- --show-only templates/crds.yaml \
- | kubectl apply --server-side -f -📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Install CRDs first (PackageSource CR in the chart depends on them) | |
| helm template installer packages/core/installer \ | |
| --namespace cozy-system \ | |
| --show-only templates/crds.yaml \ | |
| | kubectl apply --server-side -f - | |
| # Install CRDs first (PackageSource CR in the chart depends on them) | |
| helm template installer packages/core/installer \ | |
| --namespace cozy-system \ | |
| --show-only templates/crds.yaml \ | |
| > /tmp/crds.yaml | |
| kubectl apply --server-side -f /tmp/crds.yaml |
🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@hack/e2e-install-cozystack.bats` around lines 11 - 15, The pipeline running
`helm template ... | kubectl apply ...` can hide a failing `helm template`
because pipefail is not enabled; update the script to ensure `helm template`
failures are detected (e.g., enable `set -o pipefail` / `set -euo pipefail` at
the top of the bats script or explicitly capture and check the exit status of
the `helm template` command before piping to `kubectl apply`) so that errors
from the `helm template` step abort the test instead of being swallowed by the
pipe.
There was a problem hiding this comment.
This is no longer relevant — the current code uses helm upgrade --install directly, not a pipeline with helm template | kubectl apply.
There was a problem hiding this comment.
@lexfrei, you're absolutely right! The current code uses helm upgrade --install directly, which properly handles errors and doesn't have the pipefail concern I raised. The review comment was based on an earlier version of the diff that used the pipeline pattern. Thank you for the clarification.
🧠 Learnings used
Learnt from: lexfrei
Repo: cozystack/cozystack PR: 2058
File: hack/e2e-apps/harbor.bats:3-45
Timestamp: 2026-02-16T18:51:06.180Z
Learning: In the cozystack project, E2E bats tests in hack/e2e-apps/ use inline cleanup (e.g., `kubectl delete` at the end of the test) without traps or teardown functions. The E2E framework runs each test in isolation and cleans up the namespace between runs, so framework-level cleanup is preferred over test-level error handling with traps.
Learnt from: lexfrei
Repo: cozystack/cozystack PR: 2010
File: packages/core/installer/values.yaml:10-26
Timestamp: 2026-02-10T10:44:34.963Z
Learning: In the cozystack/cozystack repository, the Makefile uses `helm template -s` to selectively render templates for different modes (talos, generic, hosted). Because Helm evaluates all templates regardless of `-s` filtering, using `required` checks in mode-specific templates (e.g., generic-mode templates) would break the build for other modes. Therefore, avoid suggesting `required` validation for mode-specific values in Helm templates in this repository.
Learnt from: mattia-eleuteri
Repo: cozystack/cozystack PR: 1988
File: packages/extra/external-dns/templates/external-dns.yaml:22-31
Timestamp: 2026-02-12T11:03:15.183Z
Learning: In the cozystack/cozystack repository, `valuesFrom` is not correctly supported with Cozystack's HelmRelease implementation. Inline credential values in HelmRelease specs are the expected pattern, and suggestions to move credentials to `valuesFrom` with Secret references should be avoided.
There was a problem hiding this comment.
🤖 Fix all issues with AI agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@packages/core/testing/Makefile`:
- Line 28: Add a .PHONY declaration listing all non-file targets so make won't
skip them when a same-named file exists; specifically add a line like ".PHONY:
test install-cozystack prepare-cluster test-openapi delete exec" (including any
other phony targets present) near the top of the Makefile so targets test,
install-cozystack, prepare-cluster, test-openapi, delete, exec are treated as
phony.
🧹 Nitpick comments (1)
🤖 Fix all nitpicks with AI agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@packages/core/testing/Makefile`: - Line 28: Add a .PHONY declaration listing all non-file targets so make won't skip them when a same-named file exists; specifically add a line like ".PHONY: test install-cozystack prepare-cluster test-openapi delete exec" (including any other phony targets present) near the top of the Makefile so targets test, install-cozystack, prepare-cluster, test-openapi, delete, exec are treated as phony.packages/core/testing/Makefile (1)
28-28: Consider adding.PHONYdeclarations for non-file targets.Static analysis flags
testas missing a.PHONYdeclaration. This applies to most targets in this file (test,install-cozystack,prepare-cluster,test-openapi,delete,exec, etc.). Without.PHONY, a stray file matching a target name would skip execution.🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@packages/core/testing/Makefile` at line 28, Add a .PHONY declaration listing all non-file targets so make won't skip them when a same-named file exists; specifically add a line like ".PHONY: test install-cozystack prepare-cluster test-openapi delete exec" (including any other phony targets present) near the top of the Makefile so targets test, install-cozystack, prepare-cluster, test-openapi, delete, exec are treated as phony.
| kubectl apply -f _out/assets/cozystack-operator-talos.yaml | ||
|
|
||
| # Wait for the operator deployment to become available | ||
| # Install cozy-installer chart (CRDs from crds/ are applied automatically) |
There was a problem hiding this comment.
The main problem with this is that:
Helm does not handle CRD updates. Once CRDs are installed, they are never updated again.
The usual workaround is to put CRDs into the templates folder, but in this case, once a user removes the Helm chart, it will also remove all the packages in their cluster.
Another option is to use a Helm hook that installs CRDs during upgrades.
I think the correct solution would be to distribute the CRDs separately from this chart, or let them be managed by the cozystack-operator itself, which slightly breaks the generic pattern of how this should be done.
I do have some concerns about this, though — we need to find a proper solution.
Personally, I like the idea of adding a flag to cozystack-operator, something like --install-crds, and hardcoding them the same way we did with the flux-aio manifests, using the Go embed filesystem.
Any suggestions are appreciated.
There was a problem hiding this comment.
Implemented exactly this approach:
- Added
--install-crdsflag tocozystack-operatorthat embeds CRDs via//go:embedand applies them on every startup using server-side apply (same pattern as--install-flux) - CRDs remain in Helm's
crds/directory for initialhelm install(Helm applies them before templates, needed for PackageSource CR validation) - On upgrade, the operator keeps CRDs current — Helm's limitation of never updating CRDs is addressed
- On
helm uninstall, CRDs incrds/are never deleted by Helm, so user data (Packages, PackageSources) is preserved
New code lives in internal/crdinstall/ (mirrors internal/fluxinstall/), shared YAML parsing extracted to internal/manifestutil/. hack/update-codegen.sh syncs CRDs to the embed directory, and make verify-crds ensures they stay in sync.
dosubot
bot
added
size:XL
There was a problem hiding this comment.
🧹 Nitpick comments (3)
internal/crdinstall/install.go (2)
114-121: Prefererrors.Isfor EOF checks.Lines 117 and 130 compare errors with
==againstio.EOF. Whilek8syaml.YAMLReaderreturns a bareio.EOFtoday, usingerrors.Isis more robust against future wrapping.Suggested fix
+ "errors" "fmt" "io"if err != nil { - if err == io.EOF { + if errors.Is(err, io.EOF) { break }if err := decoder.Decode(obj); err != nil { - if err == io.EOF { + if errors.Is(err, io.EOF) { continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/crdinstall/install.go` around lines 114 - 121, Replace direct equality checks against io.EOF with errors.Is checks to handle potential wrapped errors from yamlReader.Read(); update any occurrences that compare err == io.EOF (e.g., the loop reading documents via yamlReader.Read and the subsequent EOF check after unmarshalling) to use if errors.Is(err, io.EOF) and add an import for the standard "errors" package. Ensure you update both places that currently use == io.EOF and keep the existing error handling logic unchanged otherwise.
85-95: HoistpatchOptionsout of the loop.
patchOptionsis identical for every iteration; allocating it once before the loop avoids redundant allocations and the inline bool-pointer IIFE on each pass.Suggested fix
+ force := true + patchOptions := &client.PatchOptions{ + FieldManager: "cozystack-operator", + Force: &force, + } + logger.Info("Applying Cozystack CRDs", "count", len(objects)) for _, obj := range objects { - patchOptions := &client.PatchOptions{ - FieldManager: "cozystack-operator", - Force: func() *bool { b := true; return &b }(), - } - if err := k8sClient.Patch(ctx, obj, client.Apply, patchOptions); err != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/crdinstall/install.go` around lines 85 - 95, The loop creates an identical patchOptions value on each iteration which is wasteful; hoist the creation of patchOptions (including the boolean pointer used for Force) out of the for _, obj := range objects loop and reuse that single variable for all k8sClient.Patch calls, ensuring the same FieldManager and Force settings are preserved when calling k8sClient.Patch(ctx, obj, client.Apply, patchOptions).internal/crdinstall/manifests.embed.go (1)
43-44: Usefilepath.Joinfor OS filesystem paths.Line 43 uses
path.Join(POSIX-only forward slashes) for constructing the output path written viaos.WriteFile. While this works on Linux (where the operator runs),filepath.Joinis the idiomatic choice for OS filesystem paths and is whatinstall.goalready uses. Line 38 correctly usespath.Joinfor the embed.FS (which always uses forward slashes).Suggested fix
- "path" + "path/filepath" ) //go:embed manifests/*.yaml @@ -43,1 +43,1 @@ - outputPath := path.Join(dir, manifest.Name()) + outputPath := filepath.Join(dir, manifest.Name())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/crdinstall/manifests.embed.go` around lines 43 - 44, The code constructs a filesystem path using path.Join when writing embedded manifests to disk; change the outputPath assignment to use filepath.Join instead of path.Join so OS-specific separators are used before os.WriteFile is called (update the line that sets outputPath := path.Join(dir, manifest.Name()) to use filepath.Join(dir, manifest.Name()) in the same block where os.WriteFile is invoked).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/crdinstall/install.go`:
- Around line 114-121: Replace direct equality checks against io.EOF with
errors.Is checks to handle potential wrapped errors from yamlReader.Read();
update any occurrences that compare err == io.EOF (e.g., the loop reading
documents via yamlReader.Read and the subsequent EOF check after unmarshalling)
to use if errors.Is(err, io.EOF) and add an import for the standard "errors"
package. Ensure you update both places that currently use == io.EOF and keep the
existing error handling logic unchanged otherwise.
- Around line 85-95: The loop creates an identical patchOptions value on each
iteration which is wasteful; hoist the creation of patchOptions (including the
boolean pointer used for Force) out of the for _, obj := range objects loop and
reuse that single variable for all k8sClient.Patch calls, ensuring the same
FieldManager and Force settings are preserved when calling k8sClient.Patch(ctx,
obj, client.Apply, patchOptions).
In `@internal/crdinstall/manifests.embed.go`:
- Around line 43-44: The code constructs a filesystem path using path.Join when
writing embedded manifests to disk; change the outputPath assignment to use
filepath.Join instead of path.Join so OS-specific separators are used before
os.WriteFile is called (update the line that sets outputPath := path.Join(dir,
manifest.Name()) to use filepath.Join(dir, manifest.Name()) in the same block
where os.WriteFile is invoked).
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
internal/crdinstall/install.go (3)
108-108: Preferlogr.Loggerover a custom interface for the logger parameter.The
loggerparameter uses a hand-rolled interfaceinterface{ Info(string, ...interface{}) }. In controller-runtime code,logr.Loggeris the standard type and provides a richer contract (levels, error logging, keyed values). Sincelog.FromContextalready returns alogr.Logger, passing it directly would be more idiomatic.Proposed fix
+import "github.com/go-logr/logr" + -func waitForCRDsEstablished(ctx context.Context, k8sClient client.Client, objects []*unstructured.Unstructured, logger interface{ Info(string, ...interface{}) }) error { +func waitForCRDsEstablished(ctx context.Context, k8sClient client.Client, objects []*unstructured.Unstructured, logger logr.Logger) error {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/crdinstall/install.go` at line 108, Replace the custom logger interface in waitForCRDsEstablished with the standard logr.Logger: change the function signature to accept logger logr.Logger (import "github.com/go-logr/logr"), update all uses of logger.Info(...) inside waitForCRDsEstablished to work with logr, and update any call sites to pass a logr.Logger (e.g., log.FromContext(ctx) or log.FromContext(ctx).WithValues(...)) instead of the hand-rolled interface so the function uses the controller-runtime logging contract.
39-51:Installcreates an intermediatemanifestssubdirectory — is it necessary?The function creates
tmpDir, thentmpDir/manifests, passes the latter towriteEmbeddedManifests, and reads it back. SincetmpDiris already a fresh, isolated directory, the extra nesting adds no value. ThewriteEmbeddedManifestscallback writes directly into whatever directory it receives. Consider writing directly totmpDirto simplify.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/crdinstall/install.go` around lines 39 - 51, The Install function unnecessarily creates a nested manifestsDir under tmpDir; simplify by using tmpDir directly: stop creating manifestsDir (remove filepath.Join(tmpDir, "manifests") and os.MkdirAll call), pass tmpDir into writeEmbeddedManifests and any subsequent file reads instead of manifestsDir, and remove references to the manifestsDir variable so Install, tmpDir, and writeEmbeddedManifests are the only symbols involved in the temp-directory flow.
86-97:patchOptionsis re-allocated inside the loop but is identical each iteration.Hoist the allocation above the loop to avoid repeated closure + allocation per object.
Proposed fix
logger.Info("Applying Cozystack CRDs", "count", len(objects)) + force := true + patchOptions := &client.PatchOptions{ + FieldManager: "cozystack-operator", + Force: &force, + } for _, obj := range objects { - patchOptions := &client.PatchOptions{ - FieldManager: "cozystack-operator", - Force: func() *bool { b := true; return &b }(), - } - if err := k8sClient.Patch(ctx, obj, client.Apply, patchOptions); err != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/crdinstall/install.go` around lines 86 - 97, The code recreates the identical patchOptions inside each iteration of the loop; hoist the allocation of patchOptions (the &client.PatchOptions with FieldManager "cozystack-operator" and Force true) to before the for _, obj := range objects loop and reuse that single variable when calling k8sClient.Patch(ctx, obj, client.Apply, patchOptions); leave the logger.Info calls and error handling (fmt.Errorf on failure and logger.Info on success) unchanged.Makefile (2)
83-85: Consider adding-r(recursive) flag todiff.Without
-r,diffonly compares top-level files in the two directories. If subdirectories are ever introduced undercrds/ormanifests/, the check would silently miss divergence. Adding-ris a low-cost safeguard.Proposed fix
verify-crds: - `@diff` packages/core/installer/crds/ internal/crdinstall/manifests/ --exclude=.gitattributes \ + `@diff` -r packages/core/installer/crds/ internal/crdinstall/manifests/ --exclude=.gitattributes \ || (echo "ERROR: CRD manifests out of sync. Run 'make generate' to fix." && exit 1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 83 - 85, The verify-crds Makefile target uses diff to compare directories but omits the recursive flag; update the verify-crds target by adding the -r flag to the diff invocation so it recursively compares subdirectories (modify the diff command in the verify-crds rule to include -r while keeping --exclude=.gitattributes and the existing error message and exit behavior).
41-41: Glob expansion order is locale-dependent.
cat packages/core/installer/crds/*.yamlrelies on shell glob ordering, which varies byLC_COLLATE. If the concatenated file is used downstream where document order matters (e.g., diffing across environments), consider sorting explicitly withcat $(ls packages/core/installer/crds/*.yaml | sort)or similar. If order doesn't matter for this asset, this is fine as-is.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` at line 41, The shell glob in the Makefile line "cat packages/core/installer/crds/*.yaml > _out/assets/cozystack-crds.yaml" produces locale-dependent ordering; change it to explicitly sort the filenames before concatenation (e.g., list the files and pipe through sort or sort -V under a fixed locale) so the concatenated output is deterministic across environments; update the Makefile command that builds _out/assets/cozystack-crds.yaml to use a sorted file list (e.g., via ls or find | sort or setting LC_ALL=C) instead of relying on the raw glob expansion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/crdinstall/install_test.go`:
- Around line 73-79: The test "document without kind returns error" fails
because readYAMLObjects currently skips documents with an empty Kind; update
readYAMLObjects to return an error instead of silently skipping when a YAML
document is non-empty but has an empty Kind (or when all documents are skipped),
so that callers like the test in install_test.go observe a non-nil error; locate
the readYAMLObjects function in install.go and add validation to detect
non-empty docs lacking a Kind (and/or a final check for zero parsed objects) and
return a descriptive error rather than returning ([] , nil).
In `@internal/crdinstall/install.go`:
- Around line 178-212: readYAMLObjects currently skips documents where
obj.GetKind() == "" and returns nil, but tests expect an error; change the
behavior in readYAMLObjects so that when a decoded document has an empty Kind
you return a non-nil error (e.g., fmt.Errorf("document missing kind") or
similar) instead of continuing, ensuring the error is returned from
readYAMLObjects to satisfy the TestReadYAMLObjects case "document without kind
returns error".
- Around line 123-131: The code uses objects[0].GroupVersionKind() when creating
an unstructured for CRD checks (in waitForCRDsEstablished loop over crdNames),
which is fragile; instead import k8s.io/apimachinery/pkg/runtime/schema and set
the GVK explicitly to schema.GroupVersionKind{Group:"apiextensions.k8s.io",
Version:"v1", Kind:"CustomResourceDefinition"} before calling k8sClient.Get;
update the crd variable creation (unstructured.Unstructured) to use that
hardcoded GVK so the Get queries the CRD resource type regardless of objects[0]
ordering.
---
Nitpick comments:
In `@internal/crdinstall/install.go`:
- Line 108: Replace the custom logger interface in waitForCRDsEstablished with
the standard logr.Logger: change the function signature to accept logger
logr.Logger (import "github.com/go-logr/logr"), update all uses of
logger.Info(...) inside waitForCRDsEstablished to work with logr, and update any
call sites to pass a logr.Logger (e.g., log.FromContext(ctx) or
log.FromContext(ctx).WithValues(...)) instead of the hand-rolled interface so
the function uses the controller-runtime logging contract.
- Around line 39-51: The Install function unnecessarily creates a nested
manifestsDir under tmpDir; simplify by using tmpDir directly: stop creating
manifestsDir (remove filepath.Join(tmpDir, "manifests") and os.MkdirAll call),
pass tmpDir into writeEmbeddedManifests and any subsequent file reads instead of
manifestsDir, and remove references to the manifestsDir variable so Install,
tmpDir, and writeEmbeddedManifests are the only symbols involved in the
temp-directory flow.
- Around line 86-97: The code recreates the identical patchOptions inside each
iteration of the loop; hoist the allocation of patchOptions (the
&client.PatchOptions with FieldManager "cozystack-operator" and Force true) to
before the for _, obj := range objects loop and reuse that single variable when
calling k8sClient.Patch(ctx, obj, client.Apply, patchOptions); leave the
logger.Info calls and error handling (fmt.Errorf on failure and logger.Info on
success) unchanged.
In `@Makefile`:
- Around line 83-85: The verify-crds Makefile target uses diff to compare
directories but omits the recursive flag; update the verify-crds target by
adding the -r flag to the diff invocation so it recursively compares
subdirectories (modify the diff command in the verify-crds rule to include -r
while keeping --exclude=.gitattributes and the existing error message and exit
behavior).
- Line 41: The shell glob in the Makefile line "cat
packages/core/installer/crds/*.yaml > _out/assets/cozystack-crds.yaml" produces
locale-dependent ordering; change it to explicitly sort the filenames before
concatenation (e.g., list the files and pipe through sort or sort -V under a
fixed locale) so the concatenated output is deterministic across environments;
update the Makefile command that builds _out/assets/cozystack-crds.yaml to use a
sorted file list (e.g., via ls or find | sort or setting LC_ALL=C) instead of
relying on the raw glob expansion.
…k installation Replace pre-rendered static YAML application with direct helm chart installation in e2e tests. The chart directory with correct values is already present in the sandbox after pr.patch application. - Remove CRD/operator artifact upload/download from CI workflow - Remove copy-installer-manifest target from testing Makefile - Use helm upgrade --install from local chart in e2e-install-cozystack.bats Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Helm cannot validate PackageSource CR during install because the CRD is part of the same chart. Pre-apply CRDs via helm template + kubectl apply --server-side before running helm upgrade --install. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
…tall ordering Helm installs crds/ contents before processing templates, resolving the chicken-and-egg problem where PackageSource CR validation fails because its CRD hasn't been registered yet. - Move definitions/ to crds/ in the installer chart - Remove templates/crds.yaml (Helm auto-installs from crds/) - Update codegen script to write CRDs to crds/ - Replace helm template with cat for static CRD manifest generation - Remove pre-apply CRD workaround from e2e test Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
The test-cluster target references non-existent hack/e2e-cluster.bats file. Remove it and its dependency from the test target. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Embed Package and PackageSource CRDs in the operator binary using Go embed, following the same pattern as --install-flux. The operator applies CRDs at startup using server-side apply, ensuring they are updated on every operator restart/upgrade. This addresses the CRD lifecycle concern: Helm crds/ directory handles initial install, while the operator manages updates on subsequent deployments. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Add --install-crds=true to cozystack-operator container args so the operator applies embedded CRD manifests on startup via server-side apply. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
After generating CRDs to packages/core/installer/crds/, copy them to internal/crdinstall/manifests/ so the operator binary embeds the latest CRD definitions. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
… tests - Use filepath.Join instead of path.Join for OS file paths - Restrict extracted manifest permissions from 0666 to 0600 - Add unit tests for readYAMLObjects, parseManifests, and WriteEmbeddedManifests including permission verification Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
- Wait for CRDs to have Established condition after server-side apply, instead of returning immediately - Add TestInstall with fake client and interceptor to simulate CRD establishment - Add TestInstall_noManifests and TestInstall_writeManifestsFails for error paths - Fix fluxinstall/manifests.embed.go: use filepath.Join for OS paths and restrict permissions from 0666 to 0600 (same fix as crdinstall) Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
…or embed Add verify-crds target that diffs packages/core/installer/crds/ and internal/crdinstall/manifests/ to catch accidental divergence. Include it in unit-tests target so it runs in CI. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
…stall - Use explicit apiextensions.k8s.io/v1 CRD GVK in waitForCRDsEstablished instead of fragile objects[0].GroupVersionKind() - Add TestInstall_crdNotEstablished for context timeout path - Add --recursive to diff in verify-crds Makefile target - Document why both crds/ and --install-crds exist in deployment template Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
… fluxinstall Move duplicated YAML parsing (ReadYAMLObjects, ParseManifestFile) and CRD readiness check (WaitForCRDsEstablished, CollectCRDNames) into a shared internal/manifestutil package. Both crdinstall and fluxinstall now import from manifestutil instead of maintaining identical copies. Replace fluxinstall's time.Sleep(2s) after CRD apply with proper WaitForCRDsEstablished polling, matching the crdinstall behavior. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Validate that all parsed objects are CustomResourceDefinition before applying with force server-side apply. This prevents accidental application of arbitrary resources if a non-CRD file is placed in the manifests directory. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
- Check both apiVersion and kind when validating embedded CRD manifests to prevent applying objects with wrong API group - Move ctrl.SetupSignalHandler() before install phases so CRD and Flux installs respect SIGTERM instead of blocking for up to 2 minutes - Replace custom contains/searchString helpers with strings.Contains Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
The workload reconciler was refactored to use the label workloads.cozystack.io/monitor but the test still used the old workloadmonitor.cozystack.io/name label, causing the reconciler to delete the workload instead of keeping it. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
- Remove duplicate "Starting controller manager" log before install phases, keep only the one before mgr.Start() - Rename misleading test "document without kind returns error" to "decoder rejects document without kind" to match actual behavior - Document Helm uninstall CRD behavior in deployment template comment - Use --health-probe-bind-address=0 consistently with metrics-bind - Exclude all dotfiles in verify-crds diff, not just .gitattributes Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
… GVK matching CollectCRDNames now requires both apiVersion "apiextensions.k8s.io/v1" and kind "CustomResourceDefinition", consistent with the validation in crdinstall.Install. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
lexfrei
force-pushed
the
refactor/e2e-helm-install
branch
from
1b571ea to
09805ff
Compare
Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
| args: | ||
| - --leader-elect=true | ||
| - --install-flux=true | ||
| # CRDs are also in crds/ for initial helm install, but Helm never updates |
There was a problem hiding this comment.
Let's rename them from crds to definitions, and add definition to .helmignore so helm will never tocuh them, it's better to keep same things managed only by the single controller instead of many,
There was a problem hiding this comment.
Agreed internally, that we still need CRDs to install PackageSource, so let's keep it as it is, fix it later
| args: | ||
| - --leader-elect=true | ||
| - --install-flux=true | ||
| # CRDs are also in crds/ for initial helm install, but Helm never updates |
There was a problem hiding this comment.
Agreed internally, that we still need CRDs to install PackageSource, so let's keep it as it is, fix it later
2 participants
Summary
kubectl apply) with directhelm upgrade --installof thepackages/core/installerchart in E2E testspr.patchcopy-installer-manifestMakefile target and its dependenciesTest plan
install-cozystackstep succeeds withhelm upgrade --installSummary by CodeRabbit
Chores
New Features
Tests