tests(e2e): create e2e testing module based on ginkgo #1375
Conversation
dkarpele
requested review from
chengfang,
ishitasequeira,
jannfis and
pasha-codefresh
as code owners
WalkthroughAdds a Ginkgo/Gomega E2E test framework and module with fixtures, utilities, Makefile targets, and initial parallel tests; introduces a GitHub Actions Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant Runner as Ubuntu Runner
participant K3D as k3d / k3s
participant K8s as Kubernetes API
participant Kustomize as kustomize
participant Tests as Ginkgo Suite
GH->>Runner: start test-e2e job (matrix)
Runner->>Runner: checkout repo, setup Go, restore cache
Runner->>K3D: install k3d & create cluster
Runner->>Runner: build local images
Runner->>K3D: import local images into cluster
Runner->>Kustomize: build manifests and apply image patches
Runner->>K8s: apply manifests (server-side)
Runner->>Tests: invoke ginkgo runner
rect rgba(200,240,200,0.3)
Tests->>K8s: create test namespaces & ArgoCD CR (with image-updater)
Tests->>K8s: wait for Deployments/StatefulSets readiness
Tests->>K8s: create Application and ImageUpdater CRs
Tests->>K8s: poll Application and assert updated image
Tests->>K8s: cleanup namespaces/resources
end
alt failure
Runner->>GH: upload logs, ginkgo reports, pod descriptions
end
Runner->>K3D: delete cluster
GH->>GH: job ends
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1375 +/- ##
=======================================
Coverage 70.80% 70.80%
=======================================
Files 49 49
Lines 4528 4528
=======================================
Hits 3206 3206
Misses 1125 1125
Partials 197 197 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (15)
test/ginkgo/README.md (1)
97-105: Consider fixing markdown formatting issues.The static analysis tool detected inconsistent list indentation and the use of hard tabs in code blocks. While these don't affect functionality, fixing them would improve consistency:
- List indentation: Several lists use 4 or 8-space indentation where 2 spaces are expected (lines 97-105, etc.)
- Hard tabs: Code blocks contain hard tabs instead of spaces (lines 150, 157-158, 178-180, 190-192, 204-207, 232-264)
Consider running a markdown formatter to automatically fix these issues for better consistency with markdown best practices.
Also applies to: 150-150, 157-158, 178-180, 190-192, 204-207, 232-243, 251-264
.github/workflows/ci-tests.yaml (2)
184-188: Cache key may not enable cross-run reuse.Using
${{ github.run_id }}in the cache key means each workflow run creates a unique cache that won't be restored by subsequent runs. Consider using a content-based key for better cache hit rates.uses: actions/cache@v4 with: path: ~/.cache/go-build - key: ${{ runner.os }}-go-build-v1-${{ github.run_id }} + key: ${{ runner.os }}-go-build-v1-${{ hashFiles('test/ginkgo/go.sum') }} + restore-keys: | + ${{ runner.os }}-go-build-v1-
226-236: Shell conditional syntax may cause unexpected behavior.The
$()command substitution returns a string, but theif [ ... ]expects a non-empty string for true. Ifkubectl get namespacesreturns multiple lines or the grep fails, this could behave unexpectedly. Consider usinggrep -qwith exit codes instead.- if [ $(kubectl get namespaces -o=name | grep '^namespace/argocd-e2e$') ]; then + if kubectl get namespaces -o=name | grep -q '^namespace/argocd-e2e$'; then hack/pods.sh argocd-e2e > /tmp/pods.log kubectl logs -n argocd-e2e $(kubectl get po -n argocd-e2e -o=name | grep example-argocd-application-controller) > /tmp/e2e-application-controller.log kubectl logs -n argocd-e2e $(kubectl get po -n argocd-e2e -o=name | grep example-argocd-server) > /tmp/e2e-server.log kubectl describe -n argocd-e2e $(kubectl get po -n argocd-e2e -o=name | grep example-argocd-server) >> /tmp/e2e-server.log - elif [ $(kubectl get namespaces -o=name | grep '^namespace/argocd-e2e-cluster-config$') ]; then + elif kubectl get namespaces -o=name | grep -q '^namespace/argocd-e2e-cluster-config$'; then hack/pods.sh argocd-e2e-cluster-config > /tmp/pods.logtest/ginkgo/fixture/os/fixture.go (1)
23-26: Simplify nil check for byte slice.The
outputBytesvariable fromCombinedOutputwill be an empty slice (not nil) even on error. The nil check is unnecessary; you can convert directly.outputBytes, err := cmd.CombinedOutput() - - var output string - if outputBytes != nil { - output = string(outputBytes) - } + output := string(outputBytes)test/ginkgo/parallel/1-001_validate_image_updater_test.go (2)
62-75:OutputDebugOnFailruns unconditionally in AfterEach.The function name suggests it should only run on failure, but it's called on every test completion. Consider using
CurrentSpecReport().Failed()to conditionally execute.AfterEach(func() { if imageUpdater != nil { By("deleting ImageUpdater CR") Expect(k8sClient.Delete(ctx, imageUpdater)).To(Succeed()) Eventually(imageUpdater).Should(k8sFixture.NotExistByName()) } if cleanupFunc != nil { cleanupFunc() } - fixture.OutputDebugOnFail(ns) - + if CurrentSpecReport().Failed() { + fixture.OutputDebugOnFail(ns) + } })
168-183: Consider adding a descriptive failure message to the Eventually assertion.The polling logic is correct, but if this assertion times out, the failure message won't indicate what image was actually found. Adding context would improve debuggability.
Eventually(func() string { err := k8sClient.Get(ctx, client.ObjectKeyFromObject(app), app) if err != nil { return "" // Let Eventually retry on error } // Nil-safe check: The Kustomize block is only added by the Image Updater after its first run. // We must check that it and its Images field exist before trying to access them. if app.Spec.Source.Kustomize != nil && len(app.Spec.Source.Kustomize.Images) > 0 { return string(app.Spec.Source.Kustomize.Images[0]) } // Return an empty string to signify the condition is not yet met. return "" - }, "5m", "10s").Should(Equal("quay.io/dkarpele/my-guestbook:29437546.0")) + }, "5m", "10s").Should(Equal("quay.io/dkarpele/my-guestbook:29437546.0"), + "expected Application to be updated with image quay.io/dkarpele/my-guestbook:29437546.0")test/ginkgo/fixture/utils/fixtureUtils.go (1)
117-128: Interactive client config may cause issues in CI environments.
NewInteractiveDeferredLoadingClientConfigprompts on stdin for missing credentials. In CI environments where there's no TTY, this could hang or behave unexpectedly. Consider usingNewNonInteractiveDeferredLoadingClientConfiginstead.func getSystemKubeConfig() (*rest.Config, error) { overrides := clientcmd.ConfigOverrides{} loadingRules := clientcmd.NewDefaultClientConfigLoadingRules() - clientConfig := clientcmd.NewInteractiveDeferredLoadingClientConfig(loadingRules, &overrides, os.Stdin) + clientConfig := clientcmd.NewNonInteractiveDeferredLoadingClientConfig(loadingRules, &overrides) restConfig, err := clientConfig.ClientConfig() if err != nil { return nil, err } return restConfig, nil }This also allows removing the
"os"import if no longer needed elsewhere.test/ginkgo/fixture/k8s/fixture.go (1)
131-140: Simplify conditional with early return.The else block after the early return is unnecessary and can be simplified.
err = k8sClient.Get(context.Background(), client.ObjectKeyFromObject(k8sObject), k8sObject) if apierrors.IsNotFound(err) { return true - } else { - if err != nil { - GinkgoWriter.Println(err) - } - return false } + if err != nil { + GinkgoWriter.Println(err) + } + return falsetest/ginkgo/fixture/argocd/fixture.go (2)
46-69: Blockingtime.Sleepinside matcher construction may cause unexpected delays.The
time.Sleep(sleepTime)on line 57 executes when the matcher is constructed, not when it's evaluated. This means every call toBeAvailable()orBeAvailableWithCustomSleepTime()incurs the delay immediately, even if the matcher is never used or is used inside anEventuallyblock that would handle polling itself.Consider moving the sleep logic into the
WithTransformfunction so it executes during evaluation, or document that this matcher is intended to be called once per assertion.func BeAvailableWithCustomSleepTime(sleepTime time.Duration) matcher.GomegaMatcher { - - // Wait X seconds to allow operator to reconcile the ArgoCD CR, before we start checking if it's ready - // - We do this so that any previous calls to update the ArgoCD CR have been reconciled by the operator, before we wait to see if ArgoCD has become available. - // - I'm not aware of a way to do this without a sleep statement, but when we have something better we should do that instead. - time.Sleep(sleepTime) - - return fetchArgoCD(func(argocd *argov1beta1api.ArgoCD) bool { + return WithTransform(func(argocd *argov1beta1api.ArgoCD) bool { + // Wait X seconds to allow operator to reconcile the ArgoCD CR + time.Sleep(sleepTime) + + k8sClient, _, err := utils.GetE2ETestKubeClientWithError() + if err != nil { + GinkgoWriter.Println(err) + return false + } + + err = k8sClient.Get(context.Background(), client.ObjectKeyFromObject(argocd), argocd) + if err != nil { + GinkgoWriter.Println(err) + return false + } if argocd.Status.Phase != "Available" { GinkgoWriter.Println("ArgoCD status is not yet Available") return false } GinkgoWriter.Println("ArgoCD status is now", argocd.Status.Phase) return true - }) + }, BeTrue()) }
141-178:HaveConditionassumes exactly one condition exists.Line 144 checks
len(argocd.Status.Conditions) != 1, which will fail if there are multiple conditions. This seems overly restrictive—typically you'd want to find a matching condition among potentially many conditions.func HaveCondition(condition metav1.Condition) matcher.GomegaMatcher { return fetchArgoCD(func(argocd *argov1beta1api.ArgoCD) bool { - if len(argocd.Status.Conditions) != 1 { - GinkgoWriter.Println("HaveCondition: length is zero") + if len(argocd.Status.Conditions) == 0 { + GinkgoWriter.Println("HaveCondition: no conditions found") return false } - instanceCondition := argocd.Status.Conditions[0] - - GinkgoWriter.Println("HaveCondition - Message:", instanceCondition.Message, condition.Message) - if instanceCondition.Message != condition.Message { - GinkgoWriter.Println("HaveCondition: message does not match") - return false - } - - GinkgoWriter.Println("HaveCondition - Reason:", instanceCondition.Reason, condition.Reason) - if instanceCondition.Reason != condition.Reason { - GinkgoWriter.Println("HaveCondition: reason does not match") - return false - } - - GinkgoWriter.Println("HaveCondition - Status:", instanceCondition.Status, condition.Status) - if instanceCondition.Status != condition.Status { - GinkgoWriter.Println("HaveCondition: status does not match") - return false + for _, instanceCondition := range argocd.Status.Conditions { + if instanceCondition.Type == condition.Type && + instanceCondition.Status == condition.Status && + instanceCondition.Reason == condition.Reason && + instanceCondition.Message == condition.Message { + GinkgoWriter.Println("HaveCondition - matched:", instanceCondition) + return true + } } - GinkgoWriter.Println("HaveCondition - Type:", instanceCondition.Type, condition.Type) - if instanceCondition.Type != condition.Type { - GinkgoWriter.Println("HaveCondition: type does not match") - return false - } - - return true + GinkgoWriter.Println("HaveCondition - no matching condition found for:", condition) + return false }) }test/ginkgo/Makefile (2)
47-47: Add missing targets to.PHONYdeclaration.The
undeploy-argocd-operatorandtest-e2e-citargets are missing from the.PHONYdeclaration. The static analysis warnings aboutapiVersion,kind,metadata, andspecare false positives—these are YAML content within thedefineblock, not Make targets.-.PHONY: deploy-argocd-operator kustomize test-e2e-local k3d-cluster-create k3d-cluster-delete k3d-image-import +.PHONY: deploy-argocd-operator undeploy-argocd-operator kustomize test-e2e-local test-e2e-ci k3d-cluster-create k3d-cluster-delete k3d-image-import
87-99:test-e2e-localleaves k3d cluster running on test failure.If any step between
k3d-cluster-createandk3d-cluster-deletefails, the cluster will remain running, consuming resources. Consider using a trap or restructuring to ensure cleanup.test-e2e-local: ## Build local image updater, deploy operator, and run parallel e2e tests. + @cleanup() { \ + echo "--- Deleting k3d cluster ---"; \ + $(MAKE) k3d-cluster-delete || true; \ + }; \ + trap cleanup EXIT; \ @echo "--- Creating k3d cluster---" $(MAKE) k3d-cluster-create @echo "--- Building local argocd-image-updater image ---" $(MAKE) -C ../../ docker-build @echo "--- Importing image to k3d cluster ---" $(MAKE) k3d-image-import @echo "--- Deploying argocd-operator with local image-updater ---" $(MAKE) deploy-argocd-operator @echo "--- Running Parallel E2E tests ---" $(MAKE) -C ../../ e2e-tests-parallel-ginkgo - @echo "--- Deleting k3d cluster ---" - $(MAKE) k3d-cluster-deleteAlternatively, if shell traps are complex in Make, document that manual cleanup via
make k3d-cluster-deleteis required after failures.test/ginkgo/fixture/statefulset/fixture.go (2)
98-124: Redundant Kubernetes client fetch insidefetchStatefulSetwrapper.
HaveTemplateLabelWithValuealready callsfetchStatefulSet, which fetches the StatefulSet from the cluster (lines 267-277). The additionalk8sClient.Getcall inside this function (lines 107-111) is redundant—the StatefulSetssis already refreshed byfetchStatefulSet.func HaveTemplateLabelWithValue(key string, value string) matcher.GomegaMatcher { - return fetchStatefulSet(func(ss *appsv1.StatefulSet) bool { - k8sClient, _, err := utils.GetE2ETestKubeClientWithError() - if err != nil { - GinkgoWriter.Println(err) - return false - } - - err = k8sClient.Get(context.Background(), client.ObjectKeyFromObject(ss), ss) - if err != nil { - GinkgoWriter.Println("HaveTemplateLabelWithValue:", err) - return false - } - labels := ss.Spec.Template.Labels if labels == nil { GinkgoWriter.Println("HaveTemplateLabelWithValue - labels are nil") return false } GinkgoWriter.Println("HaveTemplateLabelWithValue - Key", key, "Expect:", value, "/ Have:", labels[key]) - return labels[key] == value - }) }
126-152: Same redundant fetch pattern inHaveTemplateAnnotationWithValue.Similar to
HaveTemplateLabelWithValue, the innerk8sClient.Getis redundant sincefetchStatefulSetalready refreshes the object.test/ginkgo/fixture/deployment/fixture.go (1)
187-241: Inconsistent pattern:HaveTemplateLabelWithValueandHaveTemplateAnnotationWithValuedon't usefetchDeployment.These two matchers use
WithTransformdirectly with their own client fetch logic, while all other matchers use thefetchDeploymenthelper. This creates inconsistency and code duplication.Consider refactoring to use
fetchDeploymentfor consistency:func HaveTemplateLabelWithValue(key string, value string) matcher.GomegaMatcher { - - return WithTransform(func(depl *appsv1.Deployment) bool { - k8sClient, _, err := utils.GetE2ETestKubeClientWithError() - if err != nil { - GinkgoWriter.Println(err) - return false - } - - err = k8sClient.Get(context.Background(), client.ObjectKeyFromObject(depl), depl) - if err != nil { - GinkgoWriter.Println("HaveTemplateLabelWithValue:", err) - return false - } - + return fetchDeployment(func(depl *appsv1.Deployment) bool { labels := depl.Spec.Template.Labels if labels == nil { GinkgoWriter.Println("HaveTemplateLabelWithValue - labels are nil") return false } GinkgoWriter.Println("HaveTemplateLabelWithValue - Key", key, "Expect:", value, "/ Have:", labels[key]) - return labels[key] == value - - }, BeTrue()) + }) }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
test/ginkgo/go.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
.dockerignore(1 hunks).github/workflows/ci-tests.yaml(1 hunks)Makefile(1 hunks)test/ginkgo/Makefile(1 hunks)test/ginkgo/README.md(1 hunks)test/ginkgo/fixture/application/fixture.go(1 hunks)test/ginkgo/fixture/argocd/fixture.go(1 hunks)test/ginkgo/fixture/deployment/fixture.go(1 hunks)test/ginkgo/fixture/fixture.go(1 hunks)test/ginkgo/fixture/k8s/fixture.go(1 hunks)test/ginkgo/fixture/os/fixture.go(1 hunks)test/ginkgo/fixture/statefulset/fixture.go(1 hunks)test/ginkgo/fixture/utils/fixtureUtils.go(1 hunks)test/ginkgo/go.mod(1 hunks)test/ginkgo/parallel/.gitignore(1 hunks)test/ginkgo/parallel/1-001_validate_image_updater_test.go(1 hunks)test/ginkgo/parallel/suite_test.go(1 hunks)test/ginkgo/prereqs/kustomization.yaml(1 hunks)test/ginkgo/sequential/.gitignore(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
test/ginkgo/fixture/application/fixture.go (5)
test/ginkgo/fixture/utils/fixtureUtils.go (2)
GetE2ETestKubeClientWithError(41-53)GetE2ETestKubeClient(31-39)test/ginkgo/fixture/argocd/fixture.go (1)
Update(23-44)test/ginkgo/fixture/deployment/fixture.go (1)
Update(111-127)test/ginkgo/fixture/k8s/fixture.go (1)
Update(144-161)test/ginkgo/fixture/statefulset/fixture.go (1)
Update(24-40)
test/ginkgo/fixture/utils/fixtureUtils.go (1)
api/v1alpha1/groupversion_info.go (1)
AddToScheme(35-35)
test/ginkgo/parallel/suite_test.go (1)
test/ginkgo/fixture/fixture.go (1)
EnsureSequentialCleanSlate(62-64)
test/ginkgo/parallel/1-001_validate_image_updater_test.go (4)
api/v1alpha1/imageupdater_types.go (5)
ImageUpdater(279-285)ImageUpdaterSpec(26-51)ApplicationRef(54-82)ImageConfig(108-135)CommonUpdateSettings(139-174)test/ginkgo/fixture/utils/fixtureUtils.go (1)
GetE2ETestKubeClient(31-39)test/ginkgo/fixture/argocd/fixture.go (1)
BeAvailable(47-49)registry-scanner/pkg/image/version.go (1)
UpdateStrategy(15-15)
test/ginkgo/fixture/statefulset/fixture.go (4)
test/ginkgo/fixture/application/fixture.go (1)
Update(91-108)test/ginkgo/fixture/deployment/fixture.go (11)
Update(111-127)HaveReplicas(262-267)HaveReadyReplicas(269-274)GetTemplateSpecInitContainerByName(129-140)GetTemplateSpecContainerByName(142-153)HaveTemplateSpecNodeSelector(171-185)HaveTemplateLabelWithValue(187-213)HaveTemplateAnnotationWithValue(215-241)HaveTolerations(243-253)HaveContainerCommandSubstring(290-329)HaveContainerWithEnvVar(332-355)test/ginkgo/fixture/k8s/fixture.go (1)
Update(144-161)test/ginkgo/fixture/utils/fixtureUtils.go (2)
GetE2ETestKubeClient(31-39)GetE2ETestKubeClientWithError(41-53)
test/ginkgo/fixture/deployment/fixture.go (5)
test/ginkgo/fixture/utils/fixtureUtils.go (2)
GetE2ETestKubeClientWithError(41-53)GetE2ETestKubeClient(31-39)test/ginkgo/fixture/application/fixture.go (1)
Update(91-108)test/ginkgo/fixture/argocd/fixture.go (1)
Update(23-44)test/ginkgo/fixture/k8s/fixture.go (1)
Update(144-161)test/ginkgo/fixture/statefulset/fixture.go (11)
Update(24-40)GetTemplateSpecInitContainerByName(56-67)GetTemplateSpecContainerByName(69-80)HaveTemplateSpecNodeSelector(82-96)HaveTemplateLabelWithValue(98-124)HaveTemplateAnnotationWithValue(126-152)HaveTolerations(154-162)HaveReplicas(42-47)HaveReadyReplicas(49-54)HaveContainerCommandSubstring(196-235)HaveContainerWithEnvVar(237-260)
test/ginkgo/fixture/fixture.go (7)
test/ginkgo/fixture/utils/fixtureUtils.go (2)
GetE2ETestKubeClient(31-39)GetE2ETestKubeClientWithError(41-53)test/ginkgo/fixture/application/fixture.go (1)
Update(91-108)test/ginkgo/fixture/argocd/fixture.go (1)
Update(23-44)test/ginkgo/fixture/deployment/fixture.go (1)
Update(111-127)test/ginkgo/fixture/k8s/fixture.go (1)
Update(144-161)test/ginkgo/fixture/statefulset/fixture.go (1)
Update(24-40)test/ginkgo/fixture/os/fixture.go (1)
ExecCommandWithOutputParam(15-33)
test/ginkgo/fixture/argocd/fixture.go (5)
test/ginkgo/fixture/application/fixture.go (1)
Update(91-108)test/ginkgo/fixture/deployment/fixture.go (1)
Update(111-127)test/ginkgo/fixture/k8s/fixture.go (1)
Update(144-161)test/ginkgo/fixture/statefulset/fixture.go (1)
Update(24-40)test/ginkgo/fixture/utils/fixtureUtils.go (2)
GetE2ETestKubeClient(31-39)GetE2ETestKubeClientWithError(41-53)
🪛 checkmake (0.2.2)
test/ginkgo/Makefile
[warning] 47-47: Missing required phony target "all"
(minphony)
[warning] 47-47: Missing required phony target "clean"
(minphony)
[warning] 47-47: Missing required phony target "test"
(minphony)
[warning] 20-20: Target "apiVersion" should be declared PHONY.
(phonydeclared)
[warning] 21-21: Target "kind" should be declared PHONY.
(phonydeclared)
[warning] 22-22: Target "metadata" should be declared PHONY.
(phonydeclared)
[warning] 25-25: Target "spec" should be declared PHONY.
(phonydeclared)
Makefile
[warning] 257-257: Missing required phony target "clean"
(minphony)
🪛 LanguageTool
test/ginkgo/README.md
[grammar] ~68-~68: Ensure spelling is correct
Context: ...l-ginkgo ``` ### If you are testing an argocd-operator install that is running on K8s ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~70-~70: Ensure spelling is correct
Context: ...ain Deployment in the gitops operator Namepsace. For this, you may use the NON_OLM e...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~128-~128: To form a complete sentence, be sure to include a subject.
Context: ...eate(ctx, argoCD)).To(Succeed())` - Can be read as: Expect create of argo cd CR...
(MISSING_IT_THERE)
[grammar] ~128-~128: Ensure spelling is correct
Context: ...read as: Expect create of argo cd CR to suceeed. - `Eventually(appControllerPod, "3m", ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~130-~130: To form a complete sentence, be sure to include a subject.
Context: ...Should(k8sFixture.ExistByName()) - Can be read as: Eventually the(argo cd ap...
(MISSING_IT_THERE)
[grammar] ~130-~130: Ensure spelling is correct
Context: ...er pod)should exist (within 3 minute, chekcing every 5 seconds.) -fixture.Update(arg...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~132-~132: To form a complete sentence, be sure to include a subject or ‘there’.
Context: ...e.Update(argoCD, func(){ (...)})` - Can be reas ad: Update Argo CD CR using the...
(MISSING_IT_THERE)
[style] ~339-~339: Consider using a synonym to be more concise.
Context: ...are great! they are very detailed, with lots of good examples. There are also plenty of...
(A_LOT_OF)
[style] ~341-~341: As a shorter alternative for ‘able to’, consider using “can”.
Context: ...kgo/gomega are popular enough that LLMs are able to answer questions and write code for the...
(BE_ABLE_TO)
🪛 markdownlint-cli2 (0.18.1)
test/ginkgo/README.md
97-97: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
98-98: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
99-99: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
100-100: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
101-101: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
103-103: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
104-104: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
105-105: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
113-113: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
114-114: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
115-115: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
117-117: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
118-118: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
119-119: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
128-128: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
130-130: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
132-132: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
144-144: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
145-145: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
146-146: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
147-147: Unordered list indentation
Expected: 6; Actual: 12
(MD007, ul-indent)
148-148: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
150-150: Unordered list indentation
Expected: 2; Actual: 1
(MD007, ul-indent)
150-150: Hard tabs
Column: 1
(MD010, no-hard-tabs)
157-157: Unordered list indentation
Expected: 2; Actual: 1
(MD007, ul-indent)
157-157: Hard tabs
Column: 1
(MD010, no-hard-tabs)
158-158: Unordered list indentation
Expected: 4; Actual: 2
(MD007, ul-indent)
158-158: Hard tabs
Column: 1
(MD010, no-hard-tabs)
162-162: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
178-178: Hard tabs
Column: 1
(MD010, no-hard-tabs)
179-179: Hard tabs
Column: 1
(MD010, no-hard-tabs)
180-180: Hard tabs
Column: 1
(MD010, no-hard-tabs)
190-190: Hard tabs
Column: 1
(MD010, no-hard-tabs)
191-191: Hard tabs
Column: 1
(MD010, no-hard-tabs)
192-192: Hard tabs
Column: 1
(MD010, no-hard-tabs)
204-204: Unordered list indentation
Expected: 2; Actual: 1
(MD007, ul-indent)
204-204: Hard tabs
Column: 1
(MD010, no-hard-tabs)
206-206: Unordered list indentation
Expected: 2; Actual: 1
(MD007, ul-indent)
206-206: Hard tabs
Column: 1
(MD010, no-hard-tabs)
207-207: Unordered list indentation
Expected: 2; Actual: 1
(MD007, ul-indent)
207-207: Hard tabs
Column: 1
(MD010, no-hard-tabs)
232-232: Hard tabs
Column: 1
(MD010, no-hard-tabs)
233-233: Hard tabs
Column: 1
(MD010, no-hard-tabs)
234-234: Hard tabs
Column: 1
(MD010, no-hard-tabs)
235-235: Hard tabs
Column: 1
(MD010, no-hard-tabs)
236-236: Hard tabs
Column: 1
(MD010, no-hard-tabs)
237-237: Hard tabs
Column: 1
(MD010, no-hard-tabs)
238-238: Hard tabs
Column: 1
(MD010, no-hard-tabs)
239-239: Hard tabs
Column: 1
(MD010, no-hard-tabs)
240-240: Hard tabs
Column: 1
(MD010, no-hard-tabs)
241-241: Hard tabs
Column: 1
(MD010, no-hard-tabs)
242-242: Hard tabs
Column: 1
(MD010, no-hard-tabs)
243-243: Hard tabs
Column: 1
(MD010, no-hard-tabs)
251-251: Hard tabs
Column: 1
(MD010, no-hard-tabs)
252-252: Hard tabs
Column: 1
(MD010, no-hard-tabs)
253-253: Hard tabs
Column: 1
(MD010, no-hard-tabs)
254-254: Hard tabs
Column: 1
(MD010, no-hard-tabs)
257-257: Hard tabs
Column: 1
(MD010, no-hard-tabs)
258-258: Hard tabs
Column: 1
(MD010, no-hard-tabs)
259-259: Hard tabs
Column: 1
(MD010, no-hard-tabs)
260-260: Hard tabs
Column: 1
(MD010, no-hard-tabs)
261-261: Hard tabs
Column: 1
(MD010, no-hard-tabs)
262-262: Hard tabs
Column: 1
(MD010, no-hard-tabs)
264-264: Hard tabs
Column: 4
(MD010, no-hard-tabs)
326-326: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
327-327: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
328-328: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
343-343: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Ensure registry-scanner Go modules synchronicity and run tests
- GitHub Check: Run end-to-end tests (v1.27.1)
- GitHub Check: Ensure unit tests are passing
- GitHub Check: Ensure code is correctly linted
- GitHub Check: Analyze (go)
- GitHub Check: build_image
🔇 Additional comments (23)
test/ginkgo/sequential/.gitignore (1)
1-1: LGTM!The
.gitignoreentry forginkgo.reportis appropriate for Ginkgo test suites and consistent with the parallel test directory..dockerignore (1)
14-14: LGTM!Excluding the test/ginkgo directory from Docker builds is appropriate and consistent with other test directory exclusions.
test/ginkgo/README.md (1)
1-342: Excellent comprehensive testing documentation!This documentation provides thorough guidance for running and writing E2E tests, including:
- Multiple execution modes (OLM, local, specific tests)
- Environment configuration options
- Clear parallel vs sequential guidance
- Translation guide from Kuttl to Ginkgo
- Debugging tips
The examples and detailed instructions will significantly help developers working with the test suite.
test/ginkgo/go.mod (3)
7-7: Using a release candidate version of argocd-operator.The dependency on
github.com/argoproj-labs/argocd-operator v0.17.0-rc1.0.20251127152419-b7f30e034efais a release candidate version. While this is acceptable for test code and likely intentional for testing against the latest operator features, be aware that RC versions may have stability issues or breaking changes before the final release.If this is intentional for testing purposes, this is fine. Otherwise, consider using a stable release version.
171-215: LGTM! Well-documented dependency management.The replace block appropriately:
- Documents its origin from Argo CD v3.1.9 go.mod
- Pins Kubernetes dependencies consistently to v0.33.1
- Explicitly notes CVE fixes for yaml.v2 and yaml.v3
- Follows established patterns for Kubernetes-based projects
3-3: Go version 1.24.6 is valid — no issue found.Go 1.24.6 was released on August 6, 2025, and is a legitimate minor security/bugfix update for the 1.24 series. The specified version in
test/ginkgo/go.modis correct and current.Makefile (3)
245-254: LGTM! Well-structured E2E test targets.The new Ginkgo test targets are well-designed with:
- Proper dependency on the
ginkgobinary- Appropriate timeout settings (90 minutes for E2E tests)
- Sensible parallel execution with
-procs=5- Clear echo statements for user feedback
- Correct Ginkgo flags (
-v,--trace,-r,-p)
256-259: LGTM! Ginkgo tool installation follows established patterns.The
ginkgotarget correctly:
- Follows the same pattern as other tool installations
- Pins to v2.27.2, which matches the test/ginkgo/go.mod dependency
- Uses the enhanced
go-install-toolmacro with version support
261-275: Excellent enhancement to the tool installation macro!The updated
go-install-toolmacro now properly supports versioning:
- Installs tools with version suffix (e.g.,
ginkgo-v2.27.2)- Creates symlink for convenient unversioned access
- Prevents unnecessary reinstallation when the versioned binary exists
- Handles cleanup gracefully with
|| trueThis approach allows multiple versions to coexist and makes version management more explicit.
test/ginkgo/parallel/.gitignore (1)
1-1: LGTM!Consistent with the sequential test directory's
.gitignoreentry.test/ginkgo/parallel/suite_test.go (3)
38-42: LGTM! Standard Ginkgo test suite setup.The
TestParallelSuitefunction correctly initializes the Ginkgo test framework with Gomega assertions and proper suite labeling.
44-49: LGTM! Proper suite initialization with intentional cleanup strategy.The
BeforeSuitehook correctly:
- Configures logging to integrate with Ginkgo's output
- Calls
EnsureSequentialCleanSlate()to ensure clean state before parallel tests runThe comment clarifies that using sequential cleanup (not parallel) before the parallel suite is intentional, likely to avoid race conditions during initial cleanup. While the naming might seem counterintuitive, the approach is sound.
51-52: Empty AfterSuite hook.The empty
AfterSuitehook is a placeholder for future cleanup logic if needed. This is acceptable.test/ginkgo/parallel/1-001_validate_image_updater_test.go (1)
101-107: Loop variabledeplshadowed intentionally, but pattern is verbose.The
depl := deplpattern is used to capture the loop variable. While this works, Go 1.22+ fixes loop variable capture. Consider a cleaner approach if targeting Go 1.22+.The explicit capture is safe and works across Go versions. This is acceptable.
test/ginkgo/fixture/utils/fixtureUtils.go (1)
55-113: LGTM - Comprehensive scheme registration.The scheme registration covers all necessary API groups for the E2E tests, including core Kubernetes types, Argo CD, and the Image Updater CRD. Error handling is consistent throughout.
test/ginkgo/fixture/application/fixture.go (1)
24-45: Well-structured matcher factory pattern.The
expectedConditionhelper elegantly wraps predicates with automatic object refresh from the cluster. This ensures matchers always evaluate against the latest state, which is essential for Eventually assertions.test/ginkgo/fixture/k8s/fixture.go (1)
143-160: LGTM - Consistent Update pattern.The Update helper follows the same retry-on-conflict pattern used in other fixtures (deployment, statefulset, argocd). This consistency makes the test utilities predictable and maintainable.
test/ginkgo/fixture/argocd/fixture.go (1)
203-216: LGTM!The
RunArgoCDCLIfunction correctly usesexec.Commandwith the#nosec G204annotation to acknowledge the command injection risk is mitigated by controlled input. The output logging viaGinkgoWriteris appropriate for test debugging.test/ginkgo/Makefile (1)
101-107:test-e2e-cidoesn't run tests.This target builds, imports, and deploys but doesn't execute any tests. If this is intentional (tests run separately in CI), consider adding a comment to clarify.
test/ginkgo/fixture/statefulset/fixture.go (1)
56-80: Returning pointer to loop variable copy.Both
GetTemplateSpecInitContainerByNameandGetTemplateSpecContainerByNamereturn&containerwherecontaineris a copy of the slice element. The returned pointer points to this local copy, not the original container in the StatefulSet spec. If the caller modifies the returned container, changes won't reflect in the original object.This matches the pattern in
deployment/fixture.go, so it may be intentional, but worth noting.If modification of the original is needed:
func GetTemplateSpecInitContainerByName(name string, depl appsv1.StatefulSet) *corev1.Container { - - for idx := range depl.Spec.Template.Spec.InitContainers { - - container := depl.Spec.Template.Spec.InitContainers[idx] - if container.Name == name { - return &container + for idx := range depl.Spec.Template.Spec.InitContainers { + if depl.Spec.Template.Spec.InitContainers[idx].Name == name { + return &depl.Spec.Template.Spec.InitContainers[idx] } } - return nil }test/ginkgo/fixture/fixture.go (2)
45-55: LGTM!
EnsureParallelCleanSlatecorrectly sets up Gomega defaults for parallel test execution without assuming exclusive cluster access.
332-369: LGTM!
deleteNamespaceAndVerifyproperly implements polling with context timeout and handles both delete and verification in a single loop, correctly distinguishing between not-found (success) and other errors.test/ginkgo/fixture/deployment/fixture.go (1)
129-153: Same pointer-to-copy pattern as StatefulSet fixture.
GetTemplateSpecInitContainerByNameandGetTemplateSpecContainerByNamereturn pointers to loop variable copies. This is consistent with the StatefulSet fixture, but modifications to the returned container won't affect the original.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/ci-tests.yaml (1)
177-194: Fixgo mod downloadto target the correct Go module directory.Line 180 configures
setup-gowithgo-version-file: 'test/ginkgo/go.mod'to use the e2e test module, but line 194 runsgo mod downloadfrom the repository root. This downloads dependencies for the root module, not fortest/ginkgo, causing a mismatch between the intended module and the downloaded dependencies.Apply this diff to run
go mod downloadin the correct module directory:- name: Download Go dependencies run: | - go mod download + cd test/ginkgo && go mod downloadThis ensures that the dependencies specified in
test/ginkgo/go.mod(andgo.sum) are downloaded before running the e2e tests.
🧹 Nitpick comments (1)
.github/workflows/ci-tests.yaml (1)
227-237: Use idiomatic Bash patterns for conditional checks.Lines 227 and 232 use
[ $(kubectl get namespaces...) ]to test if a namespace exists. This pattern works (checking for non-empty output) but is non-idiomatic. Use a more explicit pattern withgrep -qor rely on the kubectl exit code.Apply this diff to use a clearer, more idiomatic Bash pattern:
- if [ $(kubectl get namespaces -o=name | grep '^namespace/argocd-e2e$') ]; then + if kubectl get namespaces -o=name | grep -q '^namespace/argocd-e2e$'; thenRepeat the same pattern for the
argocd-e2e-cluster-configcheck on line 232.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci-tests.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Analyze (go)
- GitHub Check: Ensure registry-scanner Go modules synchronicity and run tests
- GitHub Check: Run end-to-end tests (v1.27.1)
- GitHub Check: Ensure unit tests are passing
- GitHub Check: build_image
- GitHub Check: Ensure code is correctly linted
🔇 Additional comments (1)
.github/workflows/ci-tests.yaml (1)
203-206: Verify that e2e test Makefile targets run in the correct context.Line 201 explicitly runs
make -C test/ginkgo test-e2e-ci, but line 206 runsmake e2e-tests-sequential-ginkgo e2e-tests-parallel-ginkgofrom the repository root. Ensure these targets are defined in the root Makefile and are intended to run from the root context. If they require module-specific setup or should run withintest/ginkgo, add-C test/ginkgoto maintain consistency.
Assisted-by: Gemini AI Signed-off-by: dkarpele <[email protected]>
dkarpele
force-pushed
the
dk-GITOPS-7468
branch
from
d0bae51 to
0c21129
Compare
dkarpele
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/ginkgo/README.md (1)
92-96: Fix typos in documentation.Several typos remain in this section:
- Line 92: "suceeed" → "succeed"
- Line 94: "chekcing" → "checking"
- Line 96: "read ad" → "read as"
🧹 Nitpick comments (9)
test/ginkgo/parallel/suite_test.go (1)
51-52: Consider removing or documenting the empty AfterSuite.The empty
AfterSuitehook serves no purpose currently. Either remove it or add a comment explaining it's a placeholder for future cleanup logic.-var _ = AfterSuite(func() { -}) +// AfterSuite is intentionally empty - cleanup is handled per-test via deferred functions +var _ = AfterSuite(func() {})test/ginkgo/fixture/os/fixture.go (1)
27-30: Simplify nil check—CombinedOutputalways returns a non-nil slice.
cmd.CombinedOutput()returns an empty byte slice ([]byte{}), notnil, when there's no output. The nil check is unnecessary.- var output string - if outputBytes != nil { - output = string(outputBytes) - } + output := string(outputBytes)test/ginkgo/fixture/utils/fixtureUtils.go (1)
121-122: Consider usingNewNonInteractiveDeferredLoadingClientConfigfor CI compatibility.
NewInteractiveDeferredLoadingClientConfigmay prompt for input when credentials are missing, which could hang in non-interactive CI environments. Consider using the non-interactive variant:loadingRules := clientcmd.NewDefaultClientConfigLoadingRules() - clientConfig := clientcmd.NewInteractiveDeferredLoadingClientConfig(loadingRules, &overrides, os.Stdin) + clientConfig := clientcmd.NewNonInteractiveDeferredLoadingClientConfig(loadingRules, &overrides)This would also allow removing the
"os"import.test/ginkgo/Makefile (1)
88-100: Consider adding cleanup-on-failure for local e2e runs.If any step fails (especially
e2e-tests-parallel-ginkgo), thek3d-cluster-deletetarget won't execute, leaving an orphaned k3d cluster. Consider using a trap or restructuring to ensure cleanup.One approach using a shell trap:
test-e2e-local: ## Build local image updater, deploy operator, and run parallel e2e tests. - @echo "--- Creating k3d cluster---" - $(MAKE) k3d-cluster-create - @echo "--- Building local argocd-image-updater image ---" - $(MAKE) -C ../../ docker-build - @echo "--- Importing image to k3d cluster ---" - $(MAKE) k3d-image-import - @echo "--- Deploying argocd-operator with local image-updater ---" - $(MAKE) deploy-argocd-operator - @echo "--- Running Parallel E2E tests ---" - $(MAKE) -C ../../ e2e-tests-parallel-ginkgo - @echo "--- Deleting k3d cluster ---" - $(MAKE) k3d-cluster-delete + @set -e; \ + trap '$(MAKE) k3d-cluster-delete' EXIT; \ + echo "--- Creating k3d cluster---"; \ + $(MAKE) k3d-cluster-create; \ + echo "--- Building local argocd-image-updater image ---"; \ + $(MAKE) -C ../../ docker-build; \ + echo "--- Importing image to k3d cluster ---"; \ + $(MAKE) k3d-image-import; \ + echo "--- Deploying argocd-operator with local image-updater ---"; \ + $(MAKE) deploy-argocd-operator; \ + echo "--- Running Parallel E2E tests ---"; \ + $(MAKE) -C ../../ e2e-tests-parallel-ginkgotest/ginkgo/fixture/statefulset/fixture.go (2)
56-80: Misleading parameter name and returns pointer to copy.
The parameter
deplsuggests a Deployment, but this is a StatefulSet fixture. Consider renaming tossfor consistency.The functions return
&containerwherecontaineris a copy of the slice element. While Go's escape analysis handles this safely, you're returning a pointer to a copy rather than to the slice element itself. If callers modify the returned container, those changes won't affect the original StatefulSet.-func GetTemplateSpecInitContainerByName(name string, depl appsv1.StatefulSet) *corev1.Container { +func GetTemplateSpecInitContainerByName(name string, ss appsv1.StatefulSet) *corev1.Container { - for idx := range depl.Spec.Template.Spec.InitContainers { + for idx := range ss.Spec.Template.Spec.InitContainers { - container := depl.Spec.Template.Spec.InitContainers[idx] - if container.Name == name { - return &container + if ss.Spec.Template.Spec.InitContainers[idx].Name == name { + return &ss.Spec.Template.Spec.InitContainers[idx] } } return nil } -func GetTemplateSpecContainerByName(name string, depl appsv1.StatefulSet) *corev1.Container { +func GetTemplateSpecContainerByName(name string, ss appsv1.StatefulSet) *corev1.Container { - for idx := range depl.Spec.Template.Spec.Containers { + for idx := range ss.Spec.Template.Spec.Containers { - container := depl.Spec.Template.Spec.Containers[idx] - if container.Name == name { - return &container + if ss.Spec.Template.Spec.Containers[idx].Name == name { + return &ss.Spec.Template.Spec.Containers[idx] } } return nil }
98-152: Redundant object fetch insidefetchStatefulSetcallback.Both
HaveTemplateLabelWithValueandHaveTemplateAnnotationWithValuecallk8sClient.Get()within the callback passed tofetchStatefulSet, butfetchStatefulSetalready fetches the latest object before invoking the callback (lines 273-277). This results in fetching the same object twice per matcher evaluation.Remove the redundant fetch:
func HaveTemplateLabelWithValue(key string, value string) matcher.GomegaMatcher { return fetchStatefulSet(func(ss *appsv1.StatefulSet) bool { - k8sClient, _, err := utils.GetE2ETestKubeClientWithError() - if err != nil { - GinkgoWriter.Println(err) - return false - } - - err = k8sClient.Get(context.Background(), client.ObjectKeyFromObject(ss), ss) - if err != nil { - GinkgoWriter.Println("HaveTemplateLabelWithValue:", err) - return false - } - labels := ss.Spec.Template.Labels if labels == nil { GinkgoWriter.Println("HaveTemplateLabelWithValue - labels are nil")Apply similar change to
HaveTemplateAnnotationWithValue.test/ginkgo/fixture/deployment/fixture.go (3)
129-153: Same issue: returns pointer to copy of container.Similar to the StatefulSet fixture, these functions return
&containerwherecontaineris a copy. Consider returning&depl.Spec.Template.Spec.Containers[idx]directly if callers need to modify the original.func GetTemplateSpecContainerByName(name string, depl appsv1.Deployment) *corev1.Container { for idx := range depl.Spec.Template.Spec.Containers { - container := depl.Spec.Template.Spec.Containers[idx] - if container.Name == name { - return &container + if depl.Spec.Template.Spec.Containers[idx].Name == name { + return &depl.Spec.Template.Spec.Containers[idx] } } return nil }
155-165: Misleading log message inHaveTemplateSpec.The log says ".spec.template.spec is nil" but it's actually checking if
NodeSelectoris nil. Additionally, this early return may cause unexpected false negatives if a valid PodSpec intentionally has no NodeSelector.Either remove the NodeSelector check or fix the log message:
func HaveTemplateSpec(podSpec corev1.PodSpec) matcher.GomegaMatcher { return fetchDeployment(func(depl *appsv1.Deployment) bool { templateSpec := depl.Spec.Template.Spec - if templateSpec.NodeSelector == nil { - GinkgoWriter.Println("HaveTemplateSpec - .spec.template.spec is nil") - return false - } GinkgoWriter.Println("HaveTemplateSpec - expected:", podSpec, "actual:", templateSpec) return reflect.DeepEqual(podSpec, templateSpec) }) }
183-237: Redundant object fetch inside callback (same issue as StatefulSet fixture).Both
HaveTemplateLabelWithValueandHaveTemplateAnnotationWithValueuseWithTransformand fetch the object themselves, but they could usefetchDeploymentwhich already handles the fetch. This creates inconsistency with other matchers in this file.Refactor to use
fetchDeploymentand remove redundant fetch:func HaveTemplateLabelWithValue(key string, value string) matcher.GomegaMatcher { - - return WithTransform(func(depl *appsv1.Deployment) bool { - k8sClient, _, err := utils.GetE2ETestKubeClientWithError() - if err != nil { - GinkgoWriter.Println(err) - return false - } - - err = k8sClient.Get(context.Background(), client.ObjectKeyFromObject(depl), depl) - if err != nil { - GinkgoWriter.Println("HaveTemplateLabelWithValue:", err) - return false - } - + return fetchDeployment(func(depl *appsv1.Deployment) bool { labels := depl.Spec.Template.Labels if labels == nil { GinkgoWriter.Println("HaveTemplateLabelWithValue - labels are nil")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
test/ginkgo/go.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
.dockerignore(1 hunks).github/workflows/ci-tests.yaml(1 hunks)Makefile(1 hunks)test/ginkgo/Makefile(1 hunks)test/ginkgo/README.md(1 hunks)test/ginkgo/fixture/application/fixture.go(1 hunks)test/ginkgo/fixture/argocd/fixture.go(1 hunks)test/ginkgo/fixture/deployment/fixture.go(1 hunks)test/ginkgo/fixture/fixture.go(1 hunks)test/ginkgo/fixture/k8s/fixture.go(1 hunks)test/ginkgo/fixture/os/fixture.go(1 hunks)test/ginkgo/fixture/statefulset/fixture.go(1 hunks)test/ginkgo/fixture/utils/fixtureUtils.go(1 hunks)test/ginkgo/go.mod(1 hunks)test/ginkgo/parallel/.gitignore(1 hunks)test/ginkgo/parallel/1-001_validate_image_updater_test.go(1 hunks)test/ginkgo/parallel/suite_test.go(1 hunks)test/ginkgo/prereqs/kustomization.yaml(1 hunks)test/ginkgo/sequential/.gitignore(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/ci-tests.yaml
🚧 Files skipped from review as they are similar to previous changes (6)
- test/ginkgo/sequential/.gitignore
- test/ginkgo/parallel/1-001_validate_image_updater_test.go
- test/ginkgo/fixture/argocd/fixture.go
- test/ginkgo/parallel/.gitignore
- .dockerignore
- test/ginkgo/fixture/application/fixture.go
🧰 Additional context used
🧬 Code graph analysis (5)
test/ginkgo/parallel/suite_test.go (1)
test/ginkgo/fixture/fixture.go (1)
EnsureSequentialCleanSlate(62-64)
test/ginkgo/fixture/k8s/fixture.go (4)
test/ginkgo/fixture/utils/fixtureUtils.go (2)
GetE2ETestKubeClientWithError(41-53)GetE2ETestKubeClient(31-39)test/ginkgo/fixture/application/fixture.go (1)
Update(91-108)test/ginkgo/fixture/deployment/fixture.go (1)
Update(111-127)test/ginkgo/fixture/statefulset/fixture.go (1)
Update(24-40)
test/ginkgo/fixture/utils/fixtureUtils.go (1)
api/v1alpha1/groupversion_info.go (1)
AddToScheme(35-35)
test/ginkgo/fixture/statefulset/fixture.go (3)
test/ginkgo/fixture/deployment/fixture.go (3)
Update(111-127)HaveReplicas(258-263)HaveReadyReplicas(265-270)test/ginkgo/fixture/k8s/fixture.go (1)
Update(144-161)test/ginkgo/fixture/utils/fixtureUtils.go (2)
GetE2ETestKubeClient(31-39)GetE2ETestKubeClientWithError(41-53)
test/ginkgo/fixture/fixture.go (7)
test/ginkgo/fixture/utils/fixtureUtils.go (2)
GetE2ETestKubeClient(31-39)GetE2ETestKubeClientWithError(41-53)test/ginkgo/fixture/application/fixture.go (1)
Update(91-108)test/ginkgo/fixture/argocd/fixture.go (1)
Update(23-44)test/ginkgo/fixture/deployment/fixture.go (1)
Update(111-127)test/ginkgo/fixture/k8s/fixture.go (1)
Update(144-161)test/ginkgo/fixture/statefulset/fixture.go (1)
Update(24-40)test/ginkgo/fixture/os/fixture.go (1)
ExecCommandWithOutputParam(16-37)
🪛 checkmake (0.2.2)
Makefile
[warning] 257-257: Missing required phony target "clean"
(minphony)
test/ginkgo/Makefile
[warning] 47-47: Missing required phony target "all"
(minphony)
[warning] 47-47: Missing required phony target "clean"
(minphony)
[warning] 47-47: Missing required phony target "test"
(minphony)
[warning] 20-20: Target "apiVersion" should be declared PHONY.
(phonydeclared)
[warning] 21-21: Target "kind" should be declared PHONY.
(phonydeclared)
[warning] 22-22: Target "metadata" should be declared PHONY.
(phonydeclared)
[warning] 25-25: Target "spec" should be declared PHONY.
(phonydeclared)
🪛 LanguageTool
test/ginkgo/README.md
[style] ~92-~92: To form a complete sentence, be sure to include a subject.
Context: ...eate(ctx, argoCD)).To(Succeed())` - Can be read as: Expect create of argo cd CR...
(MISSING_IT_THERE)
[grammar] ~92-~92: Ensure spelling is correct
Context: ...read as: Expect create of argo cd CR to suceeed. - `Eventually(appControllerPod, "3m", ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~94-~94: To form a complete sentence, be sure to include a subject.
Context: ...Should(k8sFixture.ExistByName()) - Can be read as: Eventually the(argo cd ap...
(MISSING_IT_THERE)
[grammar] ~94-~94: Ensure spelling is correct
Context: ...er pod)should exist (within 3 minute, chekcing every 5 seconds.) -fixture.Update(arg...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~96-~96: To form a complete sentence, be sure to include a subject or ‘there’.
Context: ...e.Update(argoCD, func(){ (...)})` - Can be read ad: Update Argo CD CR using the...
(MISSING_IT_THERE)
[style] ~181-~181: Consider using a synonym to be more concise.
Context: ...are great! they are very detailed, with lots of good examples. There are also plenty of...
(A_LOT_OF)
[style] ~183-~183: As a shorter alternative for ‘able to’, consider using “can”.
Context: ...kgo/gomega are popular enough that LLMs are able to answer questions and write code for the...
(BE_ABLE_TO)
🪛 markdownlint-cli2 (0.18.1)
test/ginkgo/README.md
65-65: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
66-66: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
67-67: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
68-68: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
70-70: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
71-71: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
72-72: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
79-79: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
80-80: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
81-81: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
83-83: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
84-84: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
92-92: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
94-94: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
96-96: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
106-106: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
107-107: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
108-108: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
109-109: Unordered list indentation
Expected: 6; Actual: 12
(MD007, ul-indent)
110-110: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
118-118: Unordered list indentation
Expected: 2; Actual: 1
(MD007, ul-indent)
118-118: Hard tabs
Column: 1
(MD010, no-hard-tabs)
119-119: Unordered list indentation
Expected: 4; Actual: 2
(MD007, ul-indent)
119-119: Hard tabs
Column: 1
(MD010, no-hard-tabs)
123-123: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
129-129: Hard tabs
Column: 1
(MD010, no-hard-tabs)
130-130: Hard tabs
Column: 1
(MD010, no-hard-tabs)
131-131: Hard tabs
Column: 1
(MD010, no-hard-tabs)
140-140: Hard tabs
Column: 1
(MD010, no-hard-tabs)
141-141: Hard tabs
Column: 1
(MD010, no-hard-tabs)
142-142: Hard tabs
Column: 1
(MD010, no-hard-tabs)
154-154: Unordered list indentation
Expected: 2; Actual: 1
(MD007, ul-indent)
154-154: Hard tabs
Column: 1
(MD010, no-hard-tabs)
156-156: Unordered list indentation
Expected: 2; Actual: 1
(MD007, ul-indent)
156-156: Hard tabs
Column: 1
(MD010, no-hard-tabs)
157-157: Unordered list indentation
Expected: 2; Actual: 1
(MD007, ul-indent)
157-157: Hard tabs
Column: 1
(MD010, no-hard-tabs)
169-169: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
170-170: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
171-171: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
185-185: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run end-to-end tests (v1.27.1)
- GitHub Check: build_image
- GitHub Check: Analyze (go)
🔇 Additional comments (44)
test/ginkgo/go.mod (3)
5-18: Dependencies are well-structured.The direct dependencies appropriately include the image updater, Argo CD operator, Argo CD, Ginkgo/Gomega, and required Kubernetes libraries for E2E testing.
171-180: CVE mitigations are properly documented.The replace block correctly addresses CVE-2022-3064 and CVE-2022-28948 with pinned safe versions, and the comment references the source (Argo CD v3.1.9 go.mod).
3-3: Go version 1.24.6 is valid and correctly specified.Go 1.24 was released on February 11, 2025, and Go 1.24.6 was released on August 6, 2025. This is a legitimate, supported Go version. The concern in the original review comment was based on outdated information and should be removed.
test/ginkgo/prereqs/kustomization.yaml (1)
1-4: Good use of a versioned tag for reproducibility.Using
ref=v0.17.0-rc1instead ofmasterimproves reproducibility. Note that this is a release candidate—consider updating to a stable release once v0.17.0 is available.test/ginkgo/parallel/suite_test.go (1)
38-49: Well-structured parallel test suite setup.The suite correctly registers the fail handler, sets up zap logging for Ginkgo output, and calls
EnsureSequentialCleanSlate()inBeforeSuiteto reset test state. The inline comment clarifying not to change to parallel is helpful for future maintainers.test/ginkgo/README.md (1)
1-10: Excellent documentation for the E2E testing framework.The README provides comprehensive guidance including architecture overview, running instructions, test organization (parallel vs sequential), fixture usage, and debugging tips. This will be valuable for onboarding contributors.
test/ginkgo/fixture/os/fixture.go (1)
15-19: Guard for empty arguments is correctly implemented.The function now properly validates that at least one argument is provided before accessing
cmdArgs[0], preventing potential panics.Makefile (3)
245-259: E2E test targets are well-configured.The sequential and parallel Ginkgo targets are properly structured with appropriate timeouts (90m), tracing enabled, and the parallel target uses 5 concurrent processes. The
ginkgotarget correctly installs the CLI with version pinning.
259-259: Ensure Ginkgo version matches go.mod.The Makefile pins Ginkgo CLI to
v2.27.2, which should match the version intest/ginkgo/go.mod(line 10 showsv2.27.2). Versions are consistent.
265-275: Improved go-install-tool macro with versioned binaries.The macro now creates versioned binaries (e.g.,
ginkgo-v2.27.2) and symlinks, preventing unnecessary reinstalls when the version hasn't changed. This is a good optimization for CI caching.test/ginkgo/fixture/utils/fixtureUtils.go (3)
31-39: Clean separation of error-handling strategies.
GetE2ETestKubeClientuses Gomega expectations for test contexts (fails the test on error), whileGetE2ETestKubeClientWithErrorreturns errors for callers that need custom handling.
55-114: Comprehensive scheme registration.The
getKubeClientfunction correctly registers all necessary API groups including core, apps, RBAC, CRDs, Argo CD Application CRs, Argo CD operator APIs, and the image updater's own v1alpha1 types. This aligns with the relevant code snippet showingimageUpdater.AddToSchememaps to the project's API.
129-133: Well-documented QPS/Burst tuning.The elevated QPS (50) and Burst (100) values with clear rationale help prevent rate limiting issues during cleanup in resource-constrained CI environments.
test/ginkgo/fixture/k8s/fixture.go (7)
1-19: LGTM on imports and package setup.The package structure and imports are clean. The lint ignore directives for Ginkgo/Gomega dot imports follow established convention in this codebase.
21-44: LGTM onHaveAnnotationWithValuematcher.The matcher correctly fetches the latest object state and checks annotations with proper nil handling.
46-72: LGTM onHaveLabelWithValuematcher.Consistent implementation with good diagnostic logging.
74-97: LGTM onNotHaveLabelWithValuematcher.Correctly handles the nil labels case by returning true (label doesn't exist means condition is satisfied).
99-118: LGTM onExistByNamematcher.The existence check is correct, and the previous typo issue has been addressed.
120-141: LGTM onNotExistByNamematcher.Correctly distinguishes between a not-found condition (returns true) and other errors (returns false).
143-161: LGTM on genericUpdatehelper.The retry-on-conflict pattern is consistent with other fixture implementations, and using
client.Objectprovides flexibility for arbitrary Kubernetes resources.test/ginkgo/Makefile (3)
1-49: LGTM on variable definitions and tooling setup.The variable definitions, PATCH_TEMPLATE, and tool paths are well-structured. The static analysis warnings about
apiVersion,kind,metadata, andspecbeing undeclared PHONY targets are false positives—these are part of adefineblock for YAML content, not Make targets.
51-73: LGTM on deploy/undeploy targets.The use of a temporary directory for kustomization patching and server-side apply is a clean approach.
75-85: LGTM on k3d cluster management targets.Clean and straightforward cluster lifecycle management.
test/ginkgo/fixture/statefulset/fixture.go (7)
1-21: LGTM on package setup and imports.Clean import structure following established patterns.
23-40: LGTM onUpdatehelper.Consistent retry-on-conflict pattern matching the deployment fixture.
42-54: LGTM on replica matchers.Correctly checks both replica count and
ObservedGenerationto ensure the controller has reconciled.
82-96: LGTM onHaveTemplateSpecNodeSelector.Correct use of
reflect.DeepEqualfor map comparison.
154-194: LGTM on toleration and container image matchers.Good bounds checking for container index, and proper use of
fetchStatefulSet.
196-260: LGTM on command substring and env var matchers.The command line construction and env var iteration logic is correct.
262-283: LGTM onfetchStatefulSethelper.Clean internal helper that fetches latest state before applying the predicate.
test/ginkgo/fixture/deployment/fixture.go (6)
1-22: LGTM on package setup and imports.Clean import structure following established patterns.
23-49: LGTM onGetEnvfunction.The previous issue has been addressed—now correctly returns
&currEnv.Value.
51-108: LGTM onSetEnvandRemoveEnvhelpers.Both functions correctly handle single-container deployments with proper add/replace/remove logic.
239-284: LGTM on toleration and replica matchers.Correctly use
fetchDeploymentand check both replica counts andObservedGeneration.
286-395: LGTM on remaining matchers.Container command, env var, volume, condition, and service account matchers are implemented correctly with proper bounds checking and logging.
397-418: LGTM onfetchDeploymenthelper.Consistent pattern with the StatefulSet fixture.
test/ginkgo/fixture/fixture.go (8)
1-44: LGTM on package setup and constants.Clean imports and well-documented constants for E2E test labels.
45-105: LGTM on clean slate functions.Good design separating parallel (minimal cleanup due to concurrent tests) from sequential (full cleanup possible) modes.
107-207: LGTM on namespace helpers.The comment about 13 characters is now correct, and the namespace lifecycle management with cleanup functions is well-designed.
226-312: LGTM on wait helpers.Correctly wait for generation reconciliation and ready replicas with appropriate timeouts.
332-371: LGTM ondeleteNamespaceAndVerify.The 10-minute timeout is well-justified for CI environments with ArgoCD finalizers that may delay namespace deletion.
423-524: LGTM onOutputDebugOnFailhelper.Well-designed with thread-safe deduplication to prevent repeated debug output, and respects
SKIP_DEBUG_OUTPUTenv var.
526-548: LGTM on OpenShift helpers.The unreachable code issue has been addressed, and
RunningOnOpenShiftuses a reasonable heuristic (>5 openshift.io CRDs).
550-609: LGTM on remaining helpers.
outputPodLogis correctly marked as unused, andIsUpstreamOperatorTestsreturningtrueis appropriate for the argocd-image-updater repo.
2 participants
See README.md for details
Summary by CodeRabbit
Tests
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.