feat: new e2e test for Image Updater module #1934
Conversation
WalkthroughAdded Image Updater support: an RBAC tweak (added the Changes
Sequence Diagram(s)sequenceDiagram
actor Test
participant K8s as Kubernetes
participant ArgoCD as Argo CD (with Image Updater)
participant ImageUpd as ImageUpdater Controller
participant Registry as Container Registry
Test->>K8s: Create Argo CD Application (guestbook)
Test->>K8s: Create ImageUpdater CR (NamePattern: app*)
K8s->>ArgoCD: Application manifests
rect rgba(120,200,140,0.12)
Note over ImageUpd: Detect matching Application\nResolve image tags (semver)
ImageUpd->>Registry: Query image tags
Registry-->>ImageUpd: Return newer tag (e.g., 1.0.11)
ImageUpd->>K8s: Patch Application/Kustomize with updated image
end
rect rgba(140,160,220,0.10)
Note over Test: Verification & cleanup
Test->>K8s: Poll Application Kustomize block
K8s-->>Test: Confirm image updated
Test->>K8s: Delete ImageUpdater CR / resources
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/ginkgo/parallel/1-121_validate_image_updater_test.go (1)
167-171: Consider moving cleanup to BeforeEach or AfterEach.Using
deferfor cleanup means the ImageUpdater CR deletion won't execute if the test fails before line 167 is reached. For more reliable cleanup, consider moving this to anAfterEachblock or using the namespace cleanup to handle cascade deletion.If you prefer explicit cleanup, you could refactor to:
- defer func() { - By("deleting ImageUpdater CR") - Expect(k8sClient.Delete(ctx, imageUpdater)).To(Succeed()) - Eventually(imageUpdater).Should(k8sFixture.NotExistByName()) - }()And add an
AfterEachblock:AfterEach(func() { // Cleanup logic here if imageUpdater != nil { By("deleting ImageUpdater CR") k8sClient.Delete(ctx, imageUpdater) Eventually(imageUpdater).Should(k8sFixture.NotExistByName()) } })However, since the namespace cleanup function already exists and should cascade delete all resources, the current approach may be acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
.github/workflows/ci-build.yaml(1 hunks)controllers/argocd/policyrule.go(1 hunks)go.mod(5 hunks)tests/ginkgo/fixture/utils/fixtureUtils.go(2 hunks)tests/ginkgo/parallel/1-121_validate_image_updater_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/ginkgo/parallel/1-121_validate_image_updater_test.go (6)
tests/ginkgo/fixture/fixture.go (2)
EnsureParallelCleanSlate(45-55)CreateRandomE2ETestNamespaceWithCleanupFunc(118-122)tests/ginkgo/fixture/utils/fixtureUtils.go (1)
GetE2ETestKubeClient(35-43)api/v1beta1/argocd_types.go (1)
ArgoCDImageUpdaterSpec(329-338)tests/ginkgo/fixture/argocd/fixture.go (1)
BeAvailable(47-49)tests/ginkgo/fixture/k8s/fixture.go (2)
ExistByName(101-118)NotExistByName(122-141)tests/ginkgo/fixture/application/fixture.go (2)
HaveHealthStatusCode(63-73)HaveSyncStatusCode(76-86)
⏰ 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). (4)
- GitHub Check: Run end-to-end tests (v1.27.1)
- GitHub Check: build-operator
- GitHub Check: build
- GitHub Check: Run golangci-lint and gosec
🔇 Additional comments (13)
tests/ginkgo/fixture/utils/fixtureUtils.go (2)
26-27: LGTM!The import alias for the Image Updater API is properly structured and necessary for registering the image updater types in the test scheme.
127-129: LGTM!The image updater types are correctly registered into the runtime scheme, enabling the test client to work with ImageUpdater custom resources. The error handling is consistent with existing patterns.
controllers/argocd/policyrule.go (1)
745-758: LGTM!Adding the "watch" verb for applications is essential for the Image Updater controller to observe application changes and perform image updates. The permission is appropriately scoped to the
argoproj.ioAPI group.tests/ginkgo/parallel/1-121_validate_image_updater_test.go (6)
43-56: LGTM!The test setup follows standard Ginkgo patterns and properly initializes the test environment with a clean slate before each test run.
58-77: LGTM!The ArgoCD instance creation properly enables the Image Updater with trace-level logging, which is helpful for debugging e2e test failures.
79-94: LGTM!The workload verification comprehensively checks all required components including the new Image Updater controller deployment. The timeout values are appropriate for e2e testing.
96-120: LGTM!The Application creation references the upstream ArgoCD Image Updater repository's test fixtures, which is a reasonable approach for e2e testing. The automated sync policy is necessary for testing image updates.
149-165: LGTM!The nil-safe check for the Kustomize block is excellent defensive programming, as the Image Updater only adds this field after its first run. The Eventually timeout (5m) and polling interval (10s) are appropriate for this integration test.
122-148: ImageUpdater CR configuration and image dependency verified.The guestbook image is accessible and stable. Verification confirms all tags in the 1.0.x range exist (1.0.0–1.0.11), so the semver constraint
~1.0will correctly resolve to version 1.0.11 as expected by the test. The ImageUpdater CR configuration with the NamePattern "app*" and CommonUpdateSettings are correctly applied. No issues identified.go.mod (3)
6-6: LGTM!Updating ArgoCD from v3.1.8 to v3.1.9 is a minor patch update that likely includes bug fixes and improvements to support the Image Updater integration.
34-39: LGTM!The addition of the argocd-image-updater dependency and related indirect dependencies is necessary to support the new Image Updater integration feature.
30-30: I'll search for more detailed changelog information between v1.4.0 and v1.6.0 to ensure comprehensive coverage.No breaking changes detected for sigs.k8s.io/yaml v1.6.0 upgrade.
The v1.4.0 to v1.6.0 upgrade has been successfully deployed in production environments, and the library maintains a stable v1 interface using consistent YAML-to-JSON marshaling. No compatibility issues require mitigation.
.github/workflows/ci-build.yaml (1)
98-98: Image tag confirmed accessible; no action required.The image
quay.io/cfang/argocd-image-updater:v99.9.9is accessible and exists in the quay.io repository. The tag v99.9.9 was found in the repository, confirming the test/development tag is intentionally created and available for CI use. The high version number follows standard conventions for development tags to avoid collision with production releases.
dkarpele
force-pushed
the
dk-GITOPS-8016
branch
from
a27c144 to
9cfad41
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
dkarpele
changed the title
dkarpele
changed the title
dkarpele
force-pushed
the
dk-GITOPS-8016
branch
2 times, most recently
from
c43b32d to
c7470ec
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/ginkgo/parallel/1-121_validate_image_updater_test.go (2)
69-71: Remove redundantdeferin AfterEach.The
deferkeyword on line 70 is unnecessary insideAfterEach, as the function will complete normally. Simply callcleanupFunc()directly.Apply this diff:
if cleanupFunc != nil { - defer cleanupFunc() + cleanupFunc() }
167-183: Consider making the version check more flexible.The test expects exactly version
1.0.11, but if the upstream test repository adds a newer1.0.xversion, this assertion will fail since the semver constraint~1.0allows any version>=1.0.0 <1.1.0.Consider either:
- Using a version range check instead of exact equality
- Pinning to a specific Git commit/tag in the ImageUpdater spec
- Adding a comment documenting the assumption that 1.0.11 is the latest available version
Example of a more flexible check:
- }, "5m", "10s").Should(Equal("quay.io/dkarpele/my-guestbook:1.0.11")) + }, "5m", "10s").Should(MatchRegexp(`quay\.io/dkarpele/my-guestbook:1\.0\.\d+`))Or add a clarifying comment:
+ // Note: This test assumes 1.0.11 is the latest version matching the ~1.0 semver constraint + // in the quay.io/dkarpele/my-guestbook repository. If newer 1.0.x versions are published, + // this assertion will need to be updated. }, "5m", "10s").Should(Equal("quay.io/dkarpele/my-guestbook:1.0.11"))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
controllers/argocd/policyrule.go(1 hunks)go.mod(4 hunks)tests/ginkgo/fixture/utils/fixtureUtils.go(2 hunks)tests/ginkgo/parallel/1-121_validate_image_updater_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/ginkgo/fixture/utils/fixtureUtils.go
🧰 Additional context used
🧬 Code graph analysis (1)
tests/ginkgo/parallel/1-121_validate_image_updater_test.go (6)
tests/ginkgo/fixture/fixture.go (3)
EnsureParallelCleanSlate(45-55)OutputDebugOnFail(425-522)CreateRandomE2ETestNamespaceWithCleanupFunc(118-122)tests/ginkgo/fixture/utils/fixtureUtils.go (1)
GetE2ETestKubeClient(35-43)tests/ginkgo/fixture/k8s/fixture.go (2)
NotExistByName(122-141)ExistByName(101-118)api/v1beta1/argocd_types.go (1)
ArgoCDImageUpdaterSpec(329-338)tests/ginkgo/fixture/argocd/fixture.go (1)
BeAvailable(47-49)tests/ginkgo/fixture/application/fixture.go (2)
HaveHealthStatusCode(63-73)HaveSyncStatusCode(76-86)
⏰ 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). (4)
- GitHub Check: build
- GitHub Check: Run golangci-lint and gosec
- GitHub Check: Run end-to-end tests (v1.27.1)
- GitHub Check: build-operator
🔇 Additional comments (2)
controllers/argocd/policyrule.go (1)
757-757: LGTM - Essential RBAC permission for Image Updater.Adding the "watch" verb enables the Image Updater controller to observe Application resource changes, which is necessary for it to detect when updates are needed.
go.mod (1)
34-34: LGTM - Core dependency for Image Updater functionality.The addition of
github.com/argoproj-labs/argocd-image-updater v1.0.0provides the necessary API types and functionality for the Image Updater integration tested in this PR.
dkarpele
force-pushed
the
dk-GITOPS-8016
branch
from
c7470ec to
3a80819
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/ginkgo/parallel/1-121_validate_image_updater_test.go (2)
140-165: Consider documenting the semver constraint syntax.The ImageUpdater uses
quay.io/dkarpele/my-guestbook:~1.0as the image name with semver strategy. The tilde constraint (~1.0) means "approximately version 1.0" (typically 1.0.x). Consider adding a brief comment explaining this syntax for maintainability.Apply this diff to add clarifying documentation:
By("creating ImageUpdater CR") updateStrategy := "semver" + // Image constraint ~1.0 means approximately version 1.0 (matches 1.0.x) imageUpdater = &imageUpdaterApi.ImageUpdater{
77-183: Consider verifying post-update application health.After the ImageUpdater modifies the Application spec, the test doesn't verify that the Application successfully syncs and remains healthy with the updated image. This could hide issues where the image update succeeds but the updated workload fails to deploy.
Consider adding a final verification step:
By("ensuring that the Application image has 1.0.11 version after update") 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(MatchRegexp(`quay\.io/dkarpele/my-guestbook:1\.0\.\d+`)) By("verifying the Application remains healthy after image update") Eventually(app, "3m", "5s").Should(applicationFixture.HaveHealthStatusCode(health.HealthStatusHealthy)) Eventually(app, "3m", "5s").Should(applicationFixture.HaveSyncStatusCode(appv1alpha1.SyncStatusCodeSynced)) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
controllers/argocd/policyrule.go(1 hunks)go.mod(4 hunks)tests/ginkgo/fixture/utils/fixtureUtils.go(2 hunks)tests/ginkgo/parallel/1-121_validate_image_updater_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- controllers/argocd/policyrule.go
🧰 Additional context used
🧬 Code graph analysis (1)
tests/ginkgo/parallel/1-121_validate_image_updater_test.go (6)
tests/ginkgo/fixture/fixture.go (3)
EnsureParallelCleanSlate(45-55)OutputDebugOnFail(425-522)CreateRandomE2ETestNamespaceWithCleanupFunc(118-122)tests/ginkgo/fixture/utils/fixtureUtils.go (1)
GetE2ETestKubeClient(35-43)tests/ginkgo/fixture/k8s/fixture.go (2)
NotExistByName(122-141)ExistByName(101-118)api/v1beta1/argocd_types.go (1)
ArgoCDImageUpdaterSpec(329-338)tests/ginkgo/fixture/argocd/fixture.go (1)
BeAvailable(47-49)tests/ginkgo/fixture/application/fixture.go (2)
HaveHealthStatusCode(63-73)HaveSyncStatusCode(76-86)
⏰ 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). (4)
- GitHub Check: Run end-to-end tests (v1.27.1)
- GitHub Check: Run golangci-lint and gosec
- GitHub Check: build-operator
- GitHub Check: build
🔇 Additional comments (9)
tests/ginkgo/fixture/utils/fixtureUtils.go (1)
26-26: LGTM! Clean integration of ImageUpdater API types into test scheme.The import alias and scheme registration follow the established pattern for other API types in this file. Proper error handling is in place.
Also applies to: 127-129
tests/ginkgo/parallel/1-121_validate_image_updater_test.go (4)
55-75: LGTM! Robust test setup and cleanup.The BeforeEach/AfterEach scaffolding properly initializes the test environment and ensures ImageUpdater CR cleanup before namespace deletion. The cleanup ordering is correct.
82-98: LGTM! ArgoCD instance configuration is appropriate for testing.The trace-level logging for the ImageUpdater will be helpful for debugging test failures. The 5-minute timeout with 5-second polling is reasonable for ArgoCD reconciliation.
100-112: LGTM! Comprehensive workload verification.The test correctly verifies all expected workloads including the new
argocd-argocd-image-updater-controllerdeployment and ensures replicas are ready before proceeding.
115-138: External repository path is accessible and actively maintained.Verification confirms the test data path exists and is actively maintained in the argocd-image-updater repository (HTTP 200). Recent commits show constructive refactoring of the test infrastructure (as of 2025-08-21), indicating the path is not at risk of removal and is considered stable enough for ongoing improvements.
go.mod (4)
34-34: LGTM! Expected dependency for ImageUpdater integration.The addition of
github.com/argoproj-labs/argocd-image-updater v1.0.0aligns with the new e2e test for the ImageUpdater module.
17-17: LGTM! Safe minor version bump.The Prometheus client library update from v1.22.0 to v1.23.0 is a minor version bump that should be backward compatible.
35-38: LGTM! Indirect dependency updates align with new integration.The new indirect dependencies and version updates are consistent with adding the argocd-image-updater integration and maintaining up-to-date transitive dependencies.
Also applies to: 42-181
30-30: No breaking changes identified; upgrade is safe.Between v1.4.0 and v1.6.0, only bug fixes, additions, and deprecations were introduced—no breaking changes. The deprecation in v1.5.0 redirects goyaml.v2/goyaml.v3 to go.yaml.in equivalents while maintaining compatibility. The upgrade to v1.6.0 does not introduce breaking changes or behavioral differences in YAML parsing/serialization.
dkarpele
force-pushed
the
dk-GITOPS-8016
branch
from
3a80819 to
764603f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
controllers/argocd/policyrule.go(1 hunks)go.mod(4 hunks)tests/ginkgo/fixture/utils/fixtureUtils.go(2 hunks)tests/ginkgo/parallel/1-121_validate_image_updater_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/ginkgo/fixture/utils/fixtureUtils.go
🧰 Additional context used
🧬 Code graph analysis (1)
tests/ginkgo/parallel/1-121_validate_image_updater_test.go (6)
tests/ginkgo/fixture/fixture.go (3)
EnsureParallelCleanSlate(45-55)OutputDebugOnFail(425-522)CreateRandomE2ETestNamespaceWithCleanupFunc(118-122)tests/ginkgo/fixture/utils/fixtureUtils.go (1)
GetE2ETestKubeClient(35-43)tests/ginkgo/fixture/k8s/fixture.go (2)
NotExistByName(122-141)ExistByName(101-118)api/v1beta1/argocd_types.go (1)
ArgoCDImageUpdaterSpec(329-338)tests/ginkgo/fixture/argocd/fixture.go (1)
BeAvailable(47-49)tests/ginkgo/fixture/application/fixture.go (2)
HaveHealthStatusCode(63-73)HaveSyncStatusCode(76-86)
⏰ 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). (4)
- GitHub Check: build
- GitHub Check: Run golangci-lint and gosec
- GitHub Check: Run end-to-end tests (v1.27.1)
- GitHub Check: build-operator
🔇 Additional comments (2)
controllers/argocd/policyrule.go (1)
757-757: LGTM - RBAC permission correctly added for Image Updater.Adding the "watch" verb for applications is necessary for the image updater controller to monitor Application resources and detect when image updates are needed.
tests/ginkgo/parallel/1-121_validate_image_updater_test.go (1)
167-183: Verify test resilience against version changes.A previous review noted that hard-coding exact version expectations (like
29437546.0) makes the test fragile if newer patch versions are published to the registry. Ifquay.io/dkarpele/my-guestbookpublishes a version like29437546.1that matches the~29437546.0semver constraint, this assertion will fail.Consider using a regex pattern to verify the version format while allowing for patch updates, or document that this is a controlled test image that won't receive updates.
Previous review suggested approaches:
- Use regex matching (e.g.,
MatchRegexp(\quay.io/dkarpele/my-guestbook:29437546.\d+`)`)- Pin to a stable test image you control
- Add a comment explaining the version dependency
Can you confirm whether the test image at
quay.io/dkarpele/my-guestbook:29437546.0is stable and won't receive newer versions, or implement one of the suggested resilience patterns?
dkarpele
force-pushed
the
dk-GITOPS-8016
branch
from
764603f to
3a79cb7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/ginkgo/parallel/1-121_validate_image_updater_test.go (1)
155-183: Consider the fragility of the hardcoded version expectation.The ImageUpdater is configured with a semver constraint
~29437546.0(line 155), which can match patch versions like 29437546.1, 29437546.2, etc. However, the test expects exactly29437546.0(line 183). If a newer patch version is published to the upstream image repository, the Image Updater will select it but the test will fail.This is similar to the concern raised in the previous review (for version 1.0.11), which still applies with the new version.
Consider one of these approaches:
- Use a regex pattern to verify the version format matches
29437546\.\d+- Pin to a stable test image you control to prevent upstream changes from breaking the test
- At minimum, add a comment acknowledging this upstream dependency
Example using a regex pattern:
- }, "5m", "10s").Should(Equal("quay.io/dkarpele/my-guestbook:29437546.0")) + }, "5m", "10s").Should(MatchRegexp(`quay\.io/dkarpele/my-guestbook:29437546\.\d+`))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
controllers/argocd/policyrule.go(1 hunks)go.mod(4 hunks)tests/ginkgo/fixture/utils/fixtureUtils.go(2 hunks)tests/ginkgo/parallel/1-121_validate_image_updater_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- controllers/argocd/policyrule.go
🧰 Additional context used
🧬 Code graph analysis (1)
tests/ginkgo/parallel/1-121_validate_image_updater_test.go (6)
tests/ginkgo/fixture/fixture.go (3)
EnsureParallelCleanSlate(45-55)OutputDebugOnFail(425-522)CreateRandomE2ETestNamespaceWithCleanupFunc(118-122)tests/ginkgo/fixture/utils/fixtureUtils.go (1)
GetE2ETestKubeClient(35-43)tests/ginkgo/fixture/k8s/fixture.go (2)
NotExistByName(122-141)ExistByName(101-118)api/v1beta1/argocd_types.go (1)
ArgoCDImageUpdaterSpec(329-338)tests/ginkgo/fixture/argocd/fixture.go (1)
BeAvailable(47-49)tests/ginkgo/fixture/application/fixture.go (2)
HaveHealthStatusCode(63-73)HaveSyncStatusCode(76-86)
⏰ 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). (4)
- GitHub Check: Run end-to-end tests (v1.27.1)
- GitHub Check: build
- GitHub Check: build-operator
- GitHub Check: Run golangci-lint and gosec
🔇 Additional comments (5)
tests/ginkgo/fixture/utils/fixtureUtils.go (1)
26-27: LGTM! Scheme registration follows existing patterns.The import alias and scheme registration for the Image Updater API types are correctly implemented, following the same pattern as other API types in this function.
Also applies to: 127-129
tests/ginkgo/parallel/1-121_validate_image_updater_test.go (2)
55-75: LGTM! Robust test setup and cleanup.The BeforeEach/AfterEach hooks properly manage test resources with explicit cleanup of the ImageUpdater CR and namespace. The conditional checks prevent nil pointer issues during cleanup.
77-138: LGTM! Solid test scaffolding.The test correctly:
- Deploys ArgoCD with Image Updater enabled
- Verifies all expected workloads reach ready state with appropriate timeouts
- Creates and validates the Application resource reaches healthy/synced status
go.mod (2)
34-34: LGTM! Image Updater dependency added as expected.The addition of
github.com/argoproj-labs/argocd-image-updater v1.0.0aligns with the PR objectives to integrate Image Updater functionality into the E2E tests.
17-178: LGTM! Dependency updates support the Image Updater integration.The updates to Prometheus, OpenTelemetry, kustomize, and other transitive dependencies appear to be required by the new Image Updater dependency and its ecosystem. The changes are consistent with standard Go module management.
Signed-off-by: dkarpele <[email protected]>
dkarpele
force-pushed
the
dk-GITOPS-8016
branch
from
3a79cb7 to
c2357d4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/ginkgo/parallel/1-121_validate_image_updater_test.go (2)
140-165: Consider documenting the non-standard version scheme.The version
29437546.0(line 155) appears to be non-standard for semantic versioning (typically uses MAJOR.MINOR.PATCH format). While the test may be correct, adding a comment explaining this version scheme would improve maintainability.Example:
By("creating ImageUpdater CR") updateStrategy := "semver" + // Note: Using timestamp-based versioning scheme (YYYYMMDD.BUILD format) imageUpdater = &imageUpdaterApi.ImageUpdater{
167-183: Acknowledge test dependency on external image registry.The test asserts an exact image tag match (line 183). As noted in previous reviews, this creates a dependency on the external registry state. If the upstream publishes a new patch version matching the
~29437546.0constraint (e.g., 29437546.1 or 29437546.2), the test will fail.The nil-safe checks (lines 175-179) are well-implemented. However, consider making the assertion more resilient by using a regex pattern or version comparison to verify the image was updated to at least the expected version.
Example of a more resilient approach:
- }, "5m", "10s").Should(Equal("quay.io/dkarpele/my-guestbook:29437546.0")) + }, "5m", "10s").Should(MatchRegexp(`quay\.io/dkarpele/my-guestbook:29437546\.\d+`))Alternatively, document this dependency with a comment explaining that the test requires the specific image version to remain available and unchanged in the registry.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
controllers/argocd/policyrule.go(1 hunks)go.mod(4 hunks)tests/ginkgo/fixture/utils/fixtureUtils.go(2 hunks)tests/ginkgo/parallel/1-121_validate_image_updater_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- controllers/argocd/policyrule.go
🧰 Additional context used
🧬 Code graph analysis (1)
tests/ginkgo/parallel/1-121_validate_image_updater_test.go (6)
tests/ginkgo/fixture/fixture.go (3)
EnsureParallelCleanSlate(45-55)OutputDebugOnFail(425-522)CreateRandomE2ETestNamespaceWithCleanupFunc(118-122)tests/ginkgo/fixture/utils/fixtureUtils.go (1)
GetE2ETestKubeClient(35-43)tests/ginkgo/fixture/k8s/fixture.go (2)
NotExistByName(122-141)ExistByName(101-118)api/v1beta1/argocd_types.go (1)
ArgoCDImageUpdaterSpec(329-338)tests/ginkgo/fixture/argocd/fixture.go (1)
BeAvailable(47-49)tests/ginkgo/fixture/application/fixture.go (2)
HaveHealthStatusCode(63-73)HaveSyncStatusCode(76-86)
🔇 Additional comments (6)
tests/ginkgo/fixture/utils/fixtureUtils.go (1)
26-27: LGTM! Proper integration of Image Updater API types.The import alias and scheme registration follow the established pattern used for other API types in this file. This enables the test fixtures to create and manipulate ImageUpdater custom resources.
Also applies to: 127-129
tests/ginkgo/parallel/1-121_validate_image_updater_test.go (4)
43-76: Well-structured test setup and teardown.The BeforeEach/AfterEach pattern properly initializes the test environment and ensures cleanup. The explicit ImageUpdater CR deletion in AfterEach (lines 63-67) is good practice for ensuring proper resource cleanup in parallel test execution.
79-98: Appropriate ArgoCD configuration for testing.Enabling trace-level logging for the Image Updater (line 89) is helpful for debugging test failures. The 5-minute timeout for reconciliation (line 98) is reasonable given the complexity of ArgoCD deployment.
100-112: Thorough workload verification.The test properly verifies all expected deployments including the Image Updater controller (line 101) and uses appropriate timeouts for readiness checks.
114-138: Application configuration is correct.The Application references well-known test data from the argocd-image-updater repository (line 124) and properly waits for healthy/synced status before proceeding.
go.mod (1)
34-39: Appropriate dependency additions for Image Updater support.The addition of
argocd-image-updater(line 34) and related transitive dependencies aligns with the PR objective of adding Image Updater e2e tests. The changes appear to be correctly generated bygo mod tidy.
3 participants
What type of PR is this?
/kind enhancement
What does this PR do / why we need it:
go get github.com/argoproj-labs/argocd-image-updaterwatchverb was added toargoproj.io/applicationsresource in accordance with https://github.com/argoproj-labs/argocd-image-updater/blob/fc7b0826e58f51834a94706e98d6f7487f693591/config/rbac/role.yaml#L48Have you updated the necessary documentation?
Which issue(s) this PR fixes:
Fixes #?
GITOPS-8016
How to test changes / Special notes to the reviewer:
Summary by CodeRabbit
New Features
Updates
Tests