Allow adding/removing manifests#485
Conversation
Signed-off-by: clyang82 <chuyang@redhat.com>
Added test coverage for ValidateManifestBundle and ValidateManifestBundleUpdate: - Tests for different number of manifests between new and old bundles - Tests for empty manifests arrays - Tests for decoding failures and invalid manifest structures - Tests for missing required fields and forbidden metadata fields - E2E tests for adding and removing manifests from workloads Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: clyang82 <chuyang@redhat.com>
WalkthroughReplaced per-object immutability and per-update bundle comparison with a manifest metadata extraction and duplicate-detection pass during manifest-bundle validation; tests updated to new validation semantics; added E2E tests that patch ManifestWork to add, remove, and replace manifests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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: 1
🤖 Fix all issues with AI agents
In `@pkg/services/validation_test.go`:
- Around line 145-149: The test loop in TestValidateManifestBundle does not
assert that an error is present when c.expectedErrorMsg is non-empty, so update
the subtest for ValidateManifestBundle: after calling err :=
ValidateManifestBundle(c.manifest) add a check that if c.expectedErrorMsg != ""
then require err != nil (or t.Fatalf/t.Errorf) and compare
strings.TrimSpace(err.Error()) to c.expectedErrorMsg; otherwise (when
expectedErrorMsg is empty) assert err == nil. Apply the same change to the
analogous loop in TestValidateUpdateManifestBundle so both tests fail when an
expected error is missing or mismatched.
🧹 Nitpick comments (2)
test/e2e/pkg/resources_test.go (2)
420-426: PreferEventuallyover fixed sleeps for manifest count assertions.
Fixed delays can be flaky in slower clusters; polling for the expected count is more reliable (same applies to the remove test below).✅ Suggested refactor
- // wait for few seconds to ensure the update is finished - <-time.After(5 * time.Second) - - updatedWork, err := sourceWorkClient.ManifestWorks(agentTestOpts.consumerName).Get(ctx, workName, metav1.GetOptions{}) - Expect(err).ShouldNot(HaveOccurred()) - Expect(len(updatedWork.Spec.Workload.Manifests)).To(Equal(2)) + Eventually(func() error { + updatedWork, err := sourceWorkClient.ManifestWorks(agentTestOpts.consumerName).Get(ctx, workName, metav1.GetOptions{}) + if err != nil { + return err + } + if len(updatedWork.Spec.Workload.Manifests) != 2 { + return fmt.Errorf("unexpected manifest count, expected 2, got %d", len(updatedWork.Spec.Workload.Manifests)) + } + return nil + }, 2*time.Minute, 2*time.Second).ShouldNot(HaveOccurred())
432-435: Make the remove‑manifest spec self‑contained (or guard the precondition).
This spec implicitly depends on the prior “add” spec. Focused runs can fail because a single‑manifest workload becomes zero after slicing. Consider ensuring at least 2 manifests (or explicitly adding one) before the removal.🛡️ Minimal guard to make the precondition explicit
newWork := work.DeepCopy() + Expect(len(newWork.Spec.Workload.Manifests)).To(BeNumerically(">=", 2)) // remove the last manifest in the workload newWork.Spec.Workload.Manifests = newWork.Spec.Workload.Manifests[:len(newWork.Spec.Workload.Manifests)-1]
Signed-off-by: clyang82 <chuyang@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/e2e/pkg/resources_test.go`:
- Around line 473-475: The test logs the wrong error variable when checking
json.Unmarshal: replace the current inline call so you capture the unmarshal
error (e.g. err := json.Unmarshal(updatedWork.Spec.Workload.Manifests[0].Raw,
&manifest)) and then use that err in the Expect(...).NotTo(HaveOccurred(),
"Error unmarshalling manifest: %v", err) message (or simply call
Expect(err).NotTo(HaveOccurred())). This fixes the diagnostic so failures in
json.Unmarshal report the correct error for the manifest variable.
🧹 Nitpick comments (3)
test/e2e/pkg/resources_test.go (3)
392-393: Consider usingEventuallyfor more reliable wait conditions.Using a fixed
time.After(5 * time.Second)sleep is less reliable than polling withEventually. Other tests in this file useEventuallywith specific conditions (e.g., checking if a resource exists). Consider replacing the fixed sleep with anEventuallyblock that verifies the work was actually created and is in the expected state.This pattern also appears at lines 423, 446, and 469.
497-500: Ignored error fromMarshalJSONand redundantObjectassignment.
The error from
obj.MarshalJSON()is silently ignored. While unlikely to fail for this simple object, ignoring errors can hide issues during test development.Setting both
manifest.Rawandmanifest.RawExtension.Objectdiffers from the existingNewManifesthelper pattern which only setsRaw. When both are set,Rawtakes precedence during serialization, making theObjectassignment redundant.♻️ Suggested improvement
- objectStr, _ := obj.MarshalJSON() + objectStr, err := obj.MarshalJSON() + if err != nil { + panic(fmt.Sprintf("failed to marshal secret manifest: %v", err)) + } manifest := workv1.Manifest{} manifest.Raw = objectStr - manifest.RawExtension.Object = obj return manifest
408-451: Test ordering dependency is intentional but consider documenting.These tests depend on sequential execution—test 2 ("remove") assumes test 1 ("add") succeeded, and test 3 ("replace") assumes test 2 succeeded. While this is appropriate for testing manifest lifecycle operations within the
Orderedcontext, consider adding a brief comment at the context level explaining this intentional dependency to help future maintainers.
| manifest := map[string]interface{}{} | ||
| Expect(json.Unmarshal(updatedWork.Spec.Workload.Manifests[0].Raw, &manifest)).NotTo(HaveOccurred(), "Error unmarshalling manifest: %v", err) | ||
| Expect(manifest["kind"]).To(Equal("Secret")) |
There was a problem hiding this comment.
Incorrect error variable in assertion message.
The err in the format string references the error from the Get() call on line 471, not the error from json.Unmarshal. If unmarshalling fails, the diagnostic message will show nil (since Get succeeded) instead of the actual unmarshal error.
🐛 Proposed fix
manifest := map[string]interface{}{}
- Expect(json.Unmarshal(updatedWork.Spec.Workload.Manifests[0].Raw, &manifest)).NotTo(HaveOccurred(), "Error unmarshalling manifest: %v", err)
+ err = json.Unmarshal(updatedWork.Spec.Workload.Manifests[0].Raw, &manifest)
+ Expect(err).NotTo(HaveOccurred(), "Error unmarshalling manifest: %v", err)
Expect(manifest["kind"]).To(Equal("Secret"))📝 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.
| manifest := map[string]interface{}{} | |
| Expect(json.Unmarshal(updatedWork.Spec.Workload.Manifests[0].Raw, &manifest)).NotTo(HaveOccurred(), "Error unmarshalling manifest: %v", err) | |
| Expect(manifest["kind"]).To(Equal("Secret")) | |
| manifest := map[string]interface{}{} | |
| err = json.Unmarshal(updatedWork.Spec.Workload.Manifests[0].Raw, &manifest) | |
| Expect(err).NotTo(HaveOccurred(), "Error unmarshalling manifest: %v", err) | |
| Expect(manifest["kind"]).To(Equal("Secret")) |
🤖 Prompt for AI Agents
In `@test/e2e/pkg/resources_test.go` around lines 473 - 475, The test logs the
wrong error variable when checking json.Unmarshal: replace the current inline
call so you capture the unmarshal error (e.g. err :=
json.Unmarshal(updatedWork.Spec.Workload.Manifests[0].Raw, &manifest)) and then
use that err in the Expect(...).NotTo(HaveOccurred(), "Error unmarshalling
manifest: %v", err) message (or simply call Expect(err).NotTo(HaveOccurred())).
This fixes the diagnostic so failures in json.Unmarshal report the correct error
for the manifest variable.
Adds comprehensive test coverage for duplicate manifest detection in ValidateManifestBundleUpdate, including tests for duplicates with same namespace, different kinds, cluster-scoped resources, and edge cases to ensure manifests with same name but different namespace or kind are correctly allowed. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: clyang82 <chuyang@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/services/validation.go (1)
91-117:⚠️ Potential issue | 🔴 CriticalThe
oldparameter is unused — tests expecting old-bundle validation will fail.The function signature accepts both
newandoldmanifest bundles, but the implementation only decodes and validates thenewbundle. Theoldparameter is completely ignored.Test cases in
validation_test.goexpect errors like"old manifest bundle is empty"(line 259) and"failed to decode old manifest bundle:..."(line 277), which will never be triggered with this implementation.🔧 Proposed fix to validate the old bundle
func ValidateManifestBundleUpdate(new, old datatypes.JSONMap) error { newManifestBundleWrapper, err := api.DecodeManifestBundle(new) if err != nil { return fmt.Errorf("failed to decode new manifest bundle: %v", err) } if newManifestBundleWrapper == nil { return fmt.Errorf("new manifest bundle is empty") } + oldManifestBundleWrapper, err := api.DecodeManifestBundle(old) + if err != nil { + return fmt.Errorf("failed to decode old manifest bundle: %v", err) + } + if oldManifestBundleWrapper == nil { + return fmt.Errorf("old manifest bundle is empty") + } + // Track seen manifests to detect duplicates seen := sets.New[string]()
🤖 Fix all issues with AI agents
In `@pkg/services/validation_test.go`:
- Around line 296-300: The test case name string is misleading—update the table
entry where name is set (the case with newPayload containing two apps/v1
Deployment manifests) to accurately reflect what it tests; e.g., change the name
from "duplicate manifests with different kinds" to "duplicate Deployment
manifests" so it matches the manifests created by the newPayload and
expectedErrorMsg in validation_test.go.
🧹 Nitpick comments (1)
pkg/services/validation.go (1)
128-137: Consider removing the unused error return.
extractManifestInfoalways returnsnilfor the error. If no validation is intended here, simplifying the signature improves clarity.♻️ Simplified signature
-func extractManifestInfo(manifest datatypes.JSONMap) (*manifestInfo, error) { +func extractManifestInfo(manifest datatypes.JSONMap) *manifestInfo { unstructuredObj := unstructured.Unstructured{Object: manifest} return &manifestInfo{ key: fmt.Sprintf("%s/%s/%s/%s", unstructuredObj.GetAPIVersion(), unstructuredObj.GetKind(), unstructuredObj.GetNamespace(), unstructuredObj.GetName()), name: unstructuredObj.GetName(), namespace: unstructuredObj.GetNamespace(), gvk: fmt.Sprintf("%s.%s", unstructuredObj.GetAPIVersion(), unstructuredObj.GetKind()), - }, nil + } }And update the call site accordingly:
- info, err := extractManifestInfo(manifest) - if err != nil { - return fmt.Errorf("failed to extract metadata from manifest at index %d: %w", i, err) - } + info := extractManifestInfo(manifest)
pkg/services/validation_test.go
Outdated
| name: "duplicate manifests with different kinds", | ||
| newPayload: newPayload(t, "{\"id\":\"75479c10-b537-4261-8058-ca2e36bac384\",\"time\":\"2024-02-05T17:31:05Z\",\"type\":\"io.open-cluster-management.works.v1alpha1.manifestbundles.spec.create_request\",\"source\":\"grpc\",\"specversion\":\"1.0\",\"datacontenttype\":\"application/json\",\"resourceid\":\"c4df9ff0-bfeb-5bc6-a0ab-4c9128d698b4\",\"clustername\":\"b288a9da-8bfe-4c82-94cc-2b48e773fc46\",\"resourceversion\":1,\"data\":{\"manifests\":[{\"apiVersion\":\"apps/v1\",\"kind\":\"Deployment\",\"metadata\":{\"name\":\"nginx\",\"namespace\":\"default\"}},{\"apiVersion\":\"apps/v1\",\"kind\":\"Deployment\",\"metadata\":{\"name\":\"nginx\",\"namespace\":\"default\"}}],\"deleteOption\":{\"propagationPolicy\":\"Foreground\"}}}"), | ||
| oldManifest: newPayload(t, "{\"id\":\"266a8cd2-2fab-4e89-9bf0-a56425ebcdf8\",\"time\":\"2024-02-05T17:31:05Z\",\"type\":\"io.open-cluster-management.works.v1alpha1.manifestbundles.spec.create_request\",\"source\":\"grpc\",\"specversion\":\"1.0\",\"datacontenttype\":\"application/json\",\"resourceid\":\"c4df9ff0-bfeb-5bc6-a0ab-4c9128d698b4\",\"clustername\":\"b288a9da-8bfe-4c82-94cc-2b48e773fc46\",\"resourceversion\":1,\"data\":{\"manifests\":[{\"apiVersion\":\"apps/v1\",\"kind\":\"Deployment\",\"metadata\":{\"name\":\"nginx\",\"namespace\":\"default\"}}],\"deleteOption\":{\"propagationPolicy\":\"Foreground\"}}}"), | ||
| expectedErrorMsg: "duplicate manifest for resource default/nginx with resource type apps/v1.Deployment", | ||
| }, |
There was a problem hiding this comment.
Misleading test case name.
The test is named "duplicate manifests with different kinds" but both manifests are apps/v1 Deployment. The name should reflect that it tests duplicate detection for the same kind (e.g., "duplicate Deployment manifests").
🤖 Prompt for AI Agents
In `@pkg/services/validation_test.go` around lines 296 - 300, The test case name
string is misleading—update the table entry where name is set (the case with
newPayload containing two apps/v1 Deployment manifests) to accurately reflect
what it tests; e.g., change the name from "duplicate manifests with different
kinds" to "duplicate Deployment manifests" so it matches the manifests created
by the newPayload and expectedErrorMsg in validation_test.go.
pkg/services/validation.go
Outdated
| seen := sets.New[string]() | ||
|
|
||
| errs := field.ErrorList{} | ||
| for i, manifest := range newManifestBundleWrapper.Manifests { |
There was a problem hiding this comment.
we may also add this check to the ValidateManifestBundle
and could we using ValidateManifestBundle instead of ValidateManifestBundleUpdate, I think they should have the same logic
Signed-off-by: clyang82 <chuyang@redhat.com>
|
LGTM |
3 participants
Files modified:
Test coverage added:
restriction)
Status: All changes have been committed with signoff and pushed to origin/allow_remove_manifest ✓