OCPCLOUD-2566: tests with VAP msgs #386
Conversation
WalkthroughAdds an AWS-only Ginkgo end-to-end test that validates VAP enforcement for Machine API Migration (MAPI), covering spec/label/annotation/providerSpec update restrictions and a bypass when authoritativeAPI is set to MachineAPI. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant T as Ginkgo Test
participant K as Kubernetes API
participant V as VAP Admission
Note over T: Setup (AWS, MAPI migration enabled)
T->>K: Create MAPI Machine (authoritativeAPI=CAPI)
K-->>T: Created
T->>K: Create corresponding CAPI Machine
K-->>T: Created
rect rgb(220,235,255)
Note right of V: Enforcement when authoritativeAPI=CAPI
T->>K: Patch protected spec/labels/annotations/providerSpec
K->>V: AdmissionReview
V-->>K: Deny with VAP error
K-->>T: Update rejected (assert)
end
rect rgb(235,255,235)
Note right of V: Switch authority -> MachineAPI
T->>K: Patch machine.authoritativeAPI -> MachineAPI
K->>V: AdmissionReview
V-->>K: Allow
K-->>T: Update succeeds
end
Note over T: Cleanup and restore authoritativeAPI if changed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting 📒 Files selected for processing (1)
🔇 Additional comments (6)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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
🧹 Nitpick comments (7)
e2e/machine_migration_vap_tests.go (7)
43-43: Avoidvar _ = Describeinside a Describe; just call Describe/ContextInside a Describe, prefer
Describe(...)orContext(...)(novar _ =). This is cleaner and avoids confusing local var decls.- var _ = Describe("VAP: machine-api-machine-vap enforcement", Ordered, func() { + Describe("VAP: machine-api-machine-vap enforcement", Ordered, func() {
53-54: Make CAPI mirror retrieval resilient
GetMachine(...)may race the mirror creation. Wrap in Eventually (or a helper that waits) to avoid flaky NotFound.Example:
- testCAPIMachine = capiframework.GetMachine(cl, testMachineName, capiframework.CAPINamespace) + Eventually(func() (*clusterv1.Machine, error) { + m := capiframework.GetMachine(cl, testMachineName, capiframework.CAPINamespace) + if m == nil { + return nil, fmt.Errorf("CAPI machine not found yet") + } + return m, nil + }, capiframework.WaitMedium, capiframework.RetryMedium).Should(Not(BeNil())) + testCAPIMachine = capiframework.GetMachine(cl, testMachineName, capiframework.CAPINamespace)Also confirm
capiframework.GetMachinealready waits; if so, this is unnecessary. Please verify.
55-63: Guard cleanup against nilsIf mirror creation fails,
testCAPIMachinecan be nil. EnsurecleanupMachineResourcestolerates nils or guard before passing.
239-256: Unused helpers; either exercise them or remove
verifyNonProtectedLabelUpdateAllowedandverifyNonProtectedAnnotationUpdateAllowedaren’t called. Keep tests lean: add small Its using them or drop to avoid dead code.Also applies to: 297-314
279-295: Deletion test must ensure the annotation exists firstIf the protected annotation isn’t present, deleting it is a no-op and may not trigger VAP. Seed it first, then attempt deletion in a separate update.
func verifyProtectedAnnotationDeletionPrevented(machine *mapiv1beta1.Machine, annotationKey, expectedError string) { By(fmt.Sprintf("Verifying that deleting protected annotation %s is prevented by VAP", annotationKey)) + // Ensure the annotation exists + Eventually(func() error { + if err := cl.Get(ctx, client.ObjectKeyFromObject(machine), machine); err != nil { + return err + } + if machine.Annotations == nil { + machine.Annotations = map[string]string{} + } + if _, ok := machine.Annotations[annotationKey]; !ok { + machine.Annotations[annotationKey] = "seed" + } + return cl.Update(ctx, machine) + }, capiframework.WaitMedium, capiframework.RetryMedium).Should(Succeed()) + Eventually(func() error { if err := cl.Get(ctx, client.ObjectKeyFromObject(machine), machine); err != nil { return err } @@ }, capiframework.WaitMedium, capiframework.RetryMedium).Should( MatchError(ContainSubstring(expectedError)), "Expected protected annotation deletion to be blocked by VAP") }
316-340: Add a test that VAP is not applied when authoritativeAPI=MachineAPIYou wrote
verifyVAPNotAppliedForMachineAPIAuthority()but never call it. Add an It under its own Context to prevent regressions.Example:
@@ - Context("spec field restrictions", func() { + Context("spec field restrictions", func() { @@ }) + Context("when authoritativeAPI is MachineAPI", func() { + It("does not apply VAP to spec updates", func() { + verifyVAPNotAppliedForMachineAPIAuthority() + }) + })Also applies to: 66-86
25-30: Be cautious with exact error text assertionsHard-coding full messages is brittle across policy/message tweaks. You already use ContainSubstring; consider matching a stable fragment (e.g., “modify spec.authoritativeAPI” / “authoritativeAPI is set to Cluster API”) or a regex to reduce churn.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
e2e/machine_migration_vap_tests.go(1 hunks)
🔇 Additional comments (2)
e2e/machine_migration_vap_tests.go (2)
32-41: AWS + feature-gate guards look goodPlatform guard and MachineAPIMigration gate are correct and reduce flakiness.
66-86: Spec-field restriction assertions are solidProviderID, taints, and spec.metadata checks are clear and use Eventually with substring match on VAP error. Nice.
Optional: After each denial, assert the field remains unchanged to prove no mutation leaked through.
Also applies to: 106-128, 130-171
|
Hi Milind, thanks for helping to check this. I found Theo has some comments in pr #355, can you help to take a look? especially use Komega instead of cl.Update, cl.Get. verifyAnnotationModificationPrevented becomes a more generic call to verifyUpdatePrevented |
|
Just to add , sometimes , machine status is going to migrating and then the changes like annotations are not blocked , but reconciled so functionally it is working , but we won't get test for it to pass, as it is not getting the error message we expect. |
There was a problem hiding this comment.
Actionable comments posted: 8
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
e2e/machine_migration_vap_tests.go(1 hunks)
🔇 Additional comments (2)
e2e/machine_migration_vap_tests.go (2)
220-248: LGTM! Good example of Komega usage.This helper correctly uses
komega.Updateandkomega.Object, demonstrating the pattern that should be followed in other helper functions.
345-363: LGTM! Correct Komega usage.This helper correctly uses
komega.Updateand demonstrates proper cleanup withmapiframework.DeleteMachines.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
e2e/machine_migration_vap_tests.go (1)
169-200: Refactor helper to use Komega (per earlier feedback).Prior review already asked for Komega over raw
cl.Get/cl.Update. Please alignverifyUpdatePrevented/verifyUpdateAllowedwith the project pattern:-func verifyUpdatePrevented(machine *mapiv1beta1.Machine, updateFunc func(*mapiv1beta1.Machine), expectedError string) { +func verifyUpdatePrevented(machine *mapiv1beta1.Machine, updateFunc func(), expectedError string) { @@ - Eventually(func() error { - freshMachine := &mapiv1beta1.Machine{} - if err := cl.Get(ctx, client.ObjectKeyFromObject(machine), freshMachine); err != nil { - return err - } - updateFunc(freshMachine) - return cl.Update(ctx, freshMachine) - }, capiframework.WaitMedium, capiframework.RetryMedium).Should( + Eventually(komega.Update(machine, updateFunc), capiframework.WaitMedium, capiframework.RetryMedium).Should(Apply the same pattern to
verifyUpdateAllowedand update call sites to pass closures without the machine argument.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
e2e/machine_migration_vap_tests.go(1 hunks)
openshift-ci
bot
added
the
do-not-merge/work-in-progress
miyadav
force-pushed
the
2566vaptests_latest
branch
from
918f2a5 to
f57cd5c
Compare
|
/unhold |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
e2e/machine_migration_vap_tests.go (3)
3-18: Stop using YAML for providerSpec; switch to encoding/json.YAML bytes in runtime.RawExtension cause JSON encoding to fail before VAP, so tests won’t observe the policy error. Replace the YAML import with encoding/json.
import ( + "encoding/json" "fmt" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/envtest/komega" - yaml "sigs.k8s.io/yaml" )
74-101: Missing coverage: spec.providerSpec update should be blockedAdd a test that mutates spec.providerSpec (e.g., instanceType) and asserts the VAP rejection.
Context("spec field restrictions", func() { It("should prevent updating spec.providerID", func() { @@ }) + It("should prevent updating spec.providerSpec", func() { + verifyAWSProviderSpecUpdatePrevented( + testMAPIMachine, + "instanceType", + testInstanceType, + vapSpecLockedMessage, + ) + }) })
183-255: providerSpec helper marshals with YAML; this breaks the update pathjson is required for RawExtension. Swap yaml.Unmarshal/Marshal to encoding/json to let the Update reach VAP.
- providerSpec := &mapiv1beta1.AWSMachineProviderConfig{} - if err := yaml.Unmarshal(freshMachine.Spec.ProviderSpec.Value.Raw, providerSpec); err != nil { + providerSpec := &mapiv1beta1.AWSMachineProviderConfig{} + if err := json.Unmarshal(freshMachine.Spec.ProviderSpec.Value.Raw, providerSpec); err != nil { return fmt.Errorf("failed to unmarshal providerSpec: %v", err) } @@ - // Marshal back to raw extension - modifiedRaw, err := yaml.Marshal(providerSpec) + // Marshal back to raw extension (JSON) + modifiedRaw, err := json.Marshal(providerSpec) if err != nil { return fmt.Errorf("failed to marshal modified providerSpec: %v", err) } freshMachine.Spec.ProviderSpec.Value = &runtime.RawExtension{Raw: modifiedRaw}
🧹 Nitpick comments (3)
e2e/machine_migration_vap_tests.go (3)
167-259: Prefer Komega here for consistency with other helpersUse komega.Update instead of manual cl.Get/cl.Update for consistency and less boilerplate. Keep JSON marshal/unmarshal as above.
-func verifyAWSProviderSpecUpdatePrevented(machine *mapiv1beta1.Machine, fieldName string, testValue interface{}, expectedError string) { - By(fmt.Sprintf("Verifying that updating AWS providerSpec.%s is prevented by VAP", fieldName)) - - Eventually(func() error { - // Get fresh copy to avoid conflicts - freshMachine := &mapiv1beta1.Machine{} - if err := cl.Get(ctx, client.ObjectKeyFromObject(machine), freshMachine); err != nil { - return err - } - // Parse the current providerSpec - if freshMachine.Spec.ProviderSpec.Value == nil { - return fmt.Errorf("providerSpec is nil") - } - providerSpec := &mapiv1beta1.AWSMachineProviderConfig{} - if err := json.Unmarshal(freshMachine.Spec.ProviderSpec.Value.Raw, providerSpec); err != nil { - return fmt.Errorf("failed to unmarshal providerSpec: %v", err) - } - // ... mutate switch ... - modifiedRaw, err := json.Marshal(providerSpec) - if err != nil { - return fmt.Errorf("failed to marshal modified providerSpec: %v", err) - } - freshMachine.Spec.ProviderSpec.Value = &runtime.RawExtension{Raw: modifiedRaw} - return cl.Update(ctx, freshMachine) - }, capiframework.WaitMedium, capiframework.RetryMedium).Should( - MatchError(ContainSubstring(expectedError)), - "Expected AWS providerSpec.%s update to be blocked by VAP", fieldName) -} +func verifyAWSProviderSpecUpdatePrevented(machine *mapiv1beta1.Machine, fieldName string, testValue interface{}, expectedError string) { + By(fmt.Sprintf("Verifying that updating AWS providerSpec.%s is prevented by VAP", fieldName)) + Eventually(komega.Update(machine, func() { + if machine.Spec.ProviderSpec.Value == nil || len(machine.Spec.ProviderSpec.Value.Raw) == 0 { + return + } + providerSpec := &mapiv1beta1.AWSMachineProviderConfig{} + if err := json.Unmarshal(machine.Spec.ProviderSpec.Value.Raw, providerSpec); err != nil { + return + } + switch fieldName { + case "instanceType": + providerSpec.InstanceType = testValue.(string) + case "amiID": + s := testValue.(string) + if providerSpec.AMI.ID == nil { + providerSpec.AMI.ID = &s + } else { + *providerSpec.AMI.ID = s + } + case "availabilityZone": + providerSpec.Placement.AvailabilityZone = testValue.(string) + case "subnetID": + s := testValue.(string) + providerSpec.Subnet = mapiv1beta1.AWSResourceReference{ID: &s} + case "securityGroups": + s := testValue.(string) + providerSpec.SecurityGroups = []mapiv1beta1.AWSResourceReference{{ID: &s}} + case "volumeSize": + if len(providerSpec.BlockDevices) > 0 && providerSpec.BlockDevices[0].EBS != nil { + v := testValue.(int64) + providerSpec.BlockDevices[0].EBS.VolumeSize = &v + } + case "volumeType": + if len(providerSpec.BlockDevices) > 0 && providerSpec.BlockDevices[0].EBS != nil { + s := testValue.(string) + providerSpec.BlockDevices[0].EBS.VolumeType = &s + } + case "encryption": + if len(providerSpec.BlockDevices) > 0 && providerSpec.BlockDevices[0].EBS != nil { + b := testValue.(bool) + providerSpec.BlockDevices[0].EBS.Encrypted = &b + } + case "tags": + tagMap := testValue.(map[string]string) + var tags []mapiv1beta1.TagSpecification + for k, v := range tagMap { + tags = append(tags, mapiv1beta1.TagSpecification{Name: k, Value: v}) + } + providerSpec.Tags = tags + default: + return + } + if raw, err := json.Marshal(providerSpec); err == nil { + machine.Spec.ProviderSpec.Value.Raw = raw + } + }), capiframework.WaitMedium, capiframework.RetryMedium).Should( + MatchError(ContainSubstring(expectedError)), + fmt.Sprintf("Expected AWS providerSpec.%s update to be blocked by VAP", fieldName), + ) +}
26-33: Unused providerSpec test constantsThese constants are currently unused. Either use them in providerSpec tests or remove to reduce noise.
102-120: Optional: also assert kubernetes.io/ label protection*You only test machine.openshift.io/; consider adding a kubernetes.io/ case for completeness.
It("should prevent modifying kubernetes.io/* labels", func() { verifyUpdatePrevented(testMAPIMachine, func() { if testMAPIMachine.Labels == nil { testMAPIMachine.Labels = make(map[string]string) } testMAPIMachine.Labels["kubernetes.io/test-label"] = testLabelValue }, vapProtectedLabelMessage) })
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
e2e/machine_migration_vap_tests.go(1 hunks)
🔇 Additional comments (1)
e2e/machine_migration_vap_tests.go (1)
150-165: Good use of Komega wrappersverifyUpdatePrevented/Allowed follow the recommended Komega pattern.
|
@miyadav can we have a proper title for the PR? (including |
miyadav
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@miyadav: This pull request references OCPCLOUD-2566 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target either version "4.21." or "openshift-4.21.", but it targets "openshift-4.20" instead. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@miyadav: This pull request references OCPCLOUD-2566 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target either version "4.21." or "openshift-4.21.", but it targets "openshift-4.20" instead. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
e2e/machine_migration_vap_tests.go (5)
74-100: Add a providerSpec update test to fully exercise spec lock.spec.providerSpec changes are part of VAP enforcement but are not covered here. You already have constants and a helper for this. Add a focused test to prevent regressions.
Apply near this context:
Context("spec field restrictions", func() { It("should prevent updating spec.providerID", func() { verifyUpdatePrevented(testMAPIMachine, func() { providerIDValue := testProviderID testMAPIMachine.Spec.ProviderID = &providerIDValue }, vapSpecLockedMessage) }) + It("should prevent updating spec.providerSpec (e.g., instanceType)", func() { + // Reuses the existing helper and constants + verifyAWSProviderSpecUpdatePrevented(testMAPIMachine, "instanceType", testInstanceType, vapSpecLockedMessage) + }) })
56-72: Optional: wait for status.authoritativeAPI=ClusterAPI to reduce flakes.Enforcement depends on status.authoritativeAPI. A short readiness wait here can make CI steadier without changing behavior.
BeforeAll(func() { // Create a MAPI machine with ClusterAPI authority to trigger VAP enforcement testMAPIMachine = createMAPIMachineWithAuthority(ctx, cl, testMachineName, mapiv1beta1.MachineAuthorityClusterAPI) // The VAP requires a matching CAPI machine as parameter testCAPIMachine = capiframework.GetMachine(cl, testMachineName, capiframework.CAPINamespace) + // Harden: wait until enforcement condition is actually met + Eventually(func() (mapiv1beta1.MachineAuthority, error) { + fresh := &mapiv1beta1.Machine{} + err := cl.Get(ctx, client.ObjectKeyFromObject(testMAPIMachine), fresh) + return fresh.Status.AuthoritativeAPI, err + }, capiframework.WaitLong, capiframework.RetryMedium).Should( + Equal(mapiv1beta1.MachineAuthorityClusterAPI), + "expect status.authoritativeAPI=ClusterAPI before enforcement checks", + )
167-259: ProviderSpec helper: avoid silent no-ops and align with Komega.
- Today, several cases can end up not modifying anything (e.g., missing BlockDevices/EBS) yet proceed to Update, risking misleading outcomes if used later.
- Style: the rest of the file uses komega.Update; consider the same for consistency.
Minimal robustness tweak (keep cl.Get/Update) — ensure a real mutation or fail fast:
func verifyAWSProviderSpecUpdatePrevented(machine *mapiv1beta1.Machine, fieldName string, testValue interface{}, expectedError string) { By(fmt.Sprintf("Verifying that updating AWS providerSpec.%s is prevented by VAP", fieldName)) Eventually(func() error { @@ - // Modify the specified field + // Modify the specified field + modified := false switch fieldName { case "instanceType": providerSpec.InstanceType = testValue.(string) + modified = true case "amiID": testValueStr := testValue.(string) if providerSpec.AMI.ID == nil { providerSpec.AMI.ID = &testValueStr } else { *providerSpec.AMI.ID = testValueStr } + modified = true case "availabilityZone": providerSpec.Placement.AvailabilityZone = testValue.(string) + modified = true case "subnetID": testValueStr := testValue.(string) providerSpec.Subnet = mapiv1beta1.AWSResourceReference{ ID: &testValueStr, } + modified = true case "securityGroups": providerSpec.SecurityGroups = []mapiv1beta1.AWSResourceReference{{ ID: &[]string{testValue.(string)}[0], }} + modified = true case "volumeSize": if len(providerSpec.BlockDevices) > 0 && providerSpec.BlockDevices[0].EBS != nil { providerSpec.BlockDevices[0].EBS.VolumeSize = &[]int64{testValue.(int64)}[0] + modified = true } case "volumeType": if len(providerSpec.BlockDevices) > 0 && providerSpec.BlockDevices[0].EBS != nil { providerSpec.BlockDevices[0].EBS.VolumeType = &[]string{testValue.(string)}[0] + modified = true } case "encryption": if len(providerSpec.BlockDevices) > 0 && providerSpec.BlockDevices[0].EBS != nil { providerSpec.BlockDevices[0].EBS.Encrypted = &[]bool{testValue.(bool)}[0] + modified = true } case "tags": @@ providerSpec.Tags = tags + modified = true - case "capacityReservationId": - providerSpec.SpotMarketOptions = &mapiv1beta1.SpotMarketOptions{ - MaxPrice: &[]string{"0.10"}[0], - } - // Note: capacityReservationId might be in different location based on AWS API version default: return fmt.Errorf("unsupported field: %s", fieldName) } + + if !modified { + return fmt.Errorf("no-op mutation for field %q (missing nested fields?)", fieldName) + }If you prefer full consistency, I can also rewrite this helper to use komega.Update and return a komega closure while still surfacing parse/mutation errors. Want me to push that version?
20-38: Prune or use the extra providerSpec constants.testInstanceType, testAMIID, testAvailabilityZone, testSubnetID, testSecurityGroupID, testVolumeSize, and testCapacityReservationID are currently unused in tests. Either use them (e.g., in the providerSpec test) or remove to keep the file lean.
261-280: Optional: avoid name collisions for the MachineAPI-authority test.To be safer under parallel CI or repeated runs, add a unique suffix.
- testMachine := createMAPIMachineWithAuthority(ctx, cl, "vap-test-mapi-authority", mapiv1beta1.MachineAuthorityMachineAPI) + uniqueName := fmt.Sprintf("vap-test-mapi-authority-%d", GinkgoParallelProcess()) + testMachine := createMAPIMachineWithAuthority(ctx, cl, uniqueName, mapiv1beta1.MachineAuthorityMachineAPI)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
e2e/machine_migration_vap_tests.go(1 hunks)
🔇 Additional comments (3)
e2e/machine_migration_vap_tests.go (3)
102-120: Protected labels: good coverage.Blocking machine.openshift.io/* while allowing non-protected labels is clearly tested.
122-140: Protected annotations: good coverage.Denial for machine.openshift.io/* and allow-list for others looks correct.
150-165: Komega-based update helpers look solid.Nice switch to komega.Update with bounded waits/retries; assertions read well.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
e2e/machine_migration_vap_tests.go (2)
26-33: Build blocker: unused constants cause compile failureThese constants are never referenced, so
go testwill fail with “declared and not used”. Either use them in tests or remove them.Apply this minimal fix (keep only those actually used today and the one we’ll use in the providerSpec test below):
const ( // Test values for MAPI machine updates testProviderID = "aws:///us-west-2a/i-test123456" testTaintValue = "test-taint-value" testLabelValue = "test-label-value" testInstanceType = "m5.xlarge" - testAMIID = "ami-test123456" - testAvailabilityZone = "us-west-2b" - testSubnetID = "subnet-test123456" - testSecurityGroupID = "sg-test123456" - testVolumeSize = int64(120) - testCapacityReservationID = "cr-test123456"
74-100: Missing providerSpec coverage — add a focused testVAP should block providerSpec edits, but there’s no test exercising it. Use the existing helper.
Context("spec field restrictions", func() { @@ It("should prevent updating spec.metadata", func() { verifyUpdatePrevented(testMAPIMachine, func() { if testMAPIMachine.Spec.ObjectMeta.Labels == nil { testMAPIMachine.Spec.ObjectMeta.Labels = make(map[string]string) } testMAPIMachine.Spec.ObjectMeta.Labels["test-spec-label"] = testLabelValue }, vapSpecLockedMessage) }) + + It("should prevent updating spec.providerSpec (instanceType)", func() { + verifyAWSProviderSpecUpdatePrevented( + testMAPIMachine, + "instanceType", + testInstanceType, + vapSpecLockedMessage, + ) + }) })
🧹 Nitpick comments (5)
e2e/machine_migration_vap_tests.go (5)
171-176: Use Komega here for consistency with the rest of the suiteThis helper still uses raw client Get/Update. Prefer Komega like your other helpers.
func verifyAWSProviderSpecUpdatePrevented(machine *mapiv1beta1.Machine, fieldName string, testValue interface{}, expectedError string) { By(fmt.Sprintf("Verifying that updating AWS providerSpec.%s is prevented by VAP", fieldName)) - - Eventually(func() error { - // Get fresh copy to avoid conflicts - freshMachine := &mapiv1beta1.Machine{} - if err := cl.Get(ctx, client.ObjectKeyFromObject(machine), freshMachine); err != nil { - return err - } + Eventually(komega.Update(machine, func() { + freshMachine := machine @@ - freshMachine.Spec.ProviderSpec.Value = &runtime.RawExtension{Raw: modifiedRaw} - return cl.Update(ctx, freshMachine) - }, capiframework.WaitMedium, capiframework.RetryMedium).Should( + freshMachine.Spec.ProviderSpec.Value = &runtime.RawExtension{Raw: modifiedRaw} + }), capiframework.WaitMedium, capiframework.RetryMedium).Should( MatchError(ContainSubstring(expectedError)), "Expected AWS providerSpec.%s update to be blocked by VAP", fieldName) }Also remove the now-unused import "sigs.k8s.io/controller-runtime/pkg/client".
Also applies to: 251-253
211-227: Guard against no-op when BlockDevices/EBS is emptyIf BlockDevices is empty or EBS is nil, the mutation won’t change anything, risking false negatives.
- case "volumeSize": - if len(providerSpec.BlockDevices) > 0 { - if providerSpec.BlockDevices[0].EBS != nil { - providerSpec.BlockDevices[0].EBS.VolumeSize = &[]int64{testValue.(int64)}[0] - } - } + case "volumeSize": + if len(providerSpec.BlockDevices) == 0 { + providerSpec.BlockDevices = []mapiv1beta1.BlockDeviceMappingSpec{{EBS: &mapiv1beta1.EBSBlockDeviceSpec{}}} + } else if providerSpec.BlockDevices[0].EBS == nil { + providerSpec.BlockDevices[0].EBS = &mapiv1beta1.EBSBlockDeviceSpec{} + } + providerSpec.BlockDevices[0].EBS.VolumeSize = &[]int64{testValue.(int64)}[0] @@ - case "volumeType": - if len(providerSpec.BlockDevices) > 0 { - if providerSpec.BlockDevices[0].EBS != nil { - providerSpec.BlockDevices[0].EBS.VolumeType = &[]string{testValue.(string)}[0] - } - } + case "volumeType": + if len(providerSpec.BlockDevices) == 0 { + providerSpec.BlockDevices = []mapiv1beta1.BlockDeviceMappingSpec{{EBS: &mapiv1beta1.EBSBlockDeviceSpec{}}} + } else if providerSpec.BlockDevices[0].EBS == nil { + providerSpec.BlockDevices[0].EBS = &mapiv1beta1.EBSBlockDeviceSpec{} + } + providerSpec.BlockDevices[0].EBS.VolumeType = &[]string{testValue.(string)}[0] @@ - case "encryption": - if len(providerSpec.BlockDevices) > 0 { - if providerSpec.BlockDevices[0].EBS != nil { - providerSpec.BlockDevices[0].EBS.Encrypted = &[]bool{testValue.(bool)}[0] - } - } + case "encryption": + if len(providerSpec.BlockDevices) == 0 { + providerSpec.BlockDevices = []mapiv1beta1.BlockDeviceMappingSpec{{EBS: &mapiv1beta1.EBSBlockDeviceSpec{}}} + } else if providerSpec.BlockDevices[0].EBS == nil { + providerSpec.BlockDevices[0].EBS = &mapiv1beta1.EBSBlockDeviceSpec{} + } + providerSpec.BlockDevices[0].EBS.Encrypted = &[]bool{testValue.(bool)}[0]
102-120: Add coverage for kubernetes.io/ protected labels*Your message asserts both machine.openshift.io/* and kubernetes.io/* are protected; only the former is tested.
Context("protected label restrictions", func() { @@ It("should allow modifying non-protected labels", func() { verifyUpdateAllowed(testMAPIMachine, func() { if testMAPIMachine.Labels == nil { testMAPIMachine.Labels = make(map[string]string) } testMAPIMachine.Labels["test-label"] = "allowed-value" }) }) + + It("should prevent modifying kubernetes.io/* labels", func() { + verifyUpdatePrevented(testMAPIMachine, func() { + if testMAPIMachine.Labels == nil { + testMAPIMachine.Labels = make(map[string]string) + } + testMAPIMachine.Labels["kubernetes.io/test-label"] = testLabelValue + }, vapProtectedLabelMessage) + }) })
207-209: Minor: avoid ephemeral pointer trick for stringsPrefer a helper for clarity, e.g., k8s.io/utils/ptr.
- providerSpec.SecurityGroups = []mapiv1beta1.AWSResourceReference{{ - ID: &[]string{testValue.(string)}[0], - }} + sg := testValue.(string) + providerSpec.SecurityGroups = []mapiv1beta1.AWSResourceReference{{ID: &sg}}
56-72: Optional: wait for authoritativeAPI readiness to de-flakeIf you hit flakes, explicitly wait until status.authoritativeAPI == ClusterAPI before assertions.
BeforeAll(func() { @@ testCAPIMachine = capiframework.GetMachine(cl, testMachineName, capiframework.CAPINamespace) + + Eventually(func() (mapiv1beta1.MachineAuthority, error) { + fresh := &mapiv1beta1.Machine{} + err := cl.Get(ctx, client.ObjectKeyFromObject(testMAPIMachine), fresh) + return fresh.Status.AuthoritativeAPI, err + }, capiframework.WaitLong, capiframework.RetryMedium).Should( + Equal(mapiv1beta1.MachineAuthorityClusterAPI), + "VAP enforces only after status.authoritativeAPI=ClusterAPI", + )
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
e2e/machine_migration_vap_tests.go(1 hunks)
🔇 Additional comments (1)
e2e/machine_migration_vap_tests.go (1)
239-241: capacityReservationId mutation looks correct nowThis fixes the earlier mismatch (was mutating Spot options). No further action.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
e2e/machine_migration_vap_tests.go (1)
238-241: Simplify the pointer creation for SecurityGroups ID.The pattern
&[]string{testValue.(string)}[0]creates a slice just to take a pointer to its first element, which is unnecessarily complex and harder to read.Apply this diff to improve clarity:
case "securityGroups": + id := testValue.(string) providerSpec.SecurityGroups = []mapiv1beta1.AWSResourceReference{{ - ID: &[]string{testValue.(string)}[0], + ID: &id, }}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
e2e/machine_migration_vap_tests.go(1 hunks)
|
/test okd-scos-e2e-aws-ovn |
1 similar comment
|
/test okd-scos-e2e-aws-ovn |
e2e/machine_migration_vap_tests.go
Outdated
| return fmt.Errorf("providerSpec is nil") | ||
| } | ||
|
|
||
| providerSpec := &mapiv1beta1.AWSMachineProviderConfig{} |
There was a problem hiding this comment.
do we have a helperfunc elsewhere that does something very similar to this? I think we added one in one of the other PRs?
(specifically one with an updateFunc() being passed in pattern)
There was a problem hiding this comment.
remember now did it in pr 355. Will update
miyadav
force-pushed
the
2566vaptests_latest
branch
from
3f188cb to
b01f005
Compare
|
@miyadav: This pull request references OCPCLOUD-2566 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target either version "4.21." or "openshift-4.21.", but it targets "openshift-4.20" instead. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
e2e/machine_migration_vap_tests.go (8)
51-53: Make test machine names unique to avoid collisions across retries/parallelism.Static name can collide across runs. Append a short random/unique suffix.
Example:
- var testMachineName = "machine-vap-test-capi-auth" + var testMachineName = fmt.Sprintf("machine-vap-test-capi-auth-%d", time.Now().UnixNano())Add
timeimport if you choose this approach.
100-107: Also block spec.metadata.annotations to complete spec-metadata coverage.You cover labels under spec.metadata; add a sibling test for annotations.
It("should prevent updating spec.metadata", func() { verifyUpdatePrevented(testMAPIMachine, func() { if testMAPIMachine.Spec.ObjectMeta.Labels == nil { testMAPIMachine.Spec.ObjectMeta.Labels = make(map[string]string) } testMAPIMachine.Spec.ObjectMeta.Labels["test-spec-label"] = testLabelValue }, vapSpecLockedMessage) }) + It("should prevent updating spec.metadata.annotations", func() { + verifyUpdatePrevented(testMAPIMachine, func() { + if testMAPIMachine.Spec.ObjectMeta.Annotations == nil { + testMAPIMachine.Spec.ObjectMeta.Annotations = make(map[string]string) + } + testMAPIMachine.Spec.ObjectMeta.Annotations["test-spec-annotation"] = "x" + }, vapSpecLockedMessage) + })
33-37: Trim the expected-message substrings to reduce brittleness.OPA/Gatekeeper messages can change slightly. Matching a shorter stable substring (e.g., “You may only modify spec.authoritativeAPI”) will make tests less fragile while preserving intent.
265-284: Use Komega here for consistency with the rest of the helpers.Refactor providerSpec enforcement helper to komega.Update; avoids manual Get/Update and aligns with other helpers.
-func verifyAWSProviderSpecUpdatePrevented(machine *mapiv1beta1.Machine, fieldName string, updateFunc func(*mapiv1beta1.AWSMachineProviderConfig), expectedError string) { - By(fmt.Sprintf("Verifying that updating AWS providerSpec.%s is prevented by VAP", fieldName)) - - Eventually(func() error { - // Get fresh copy to avoid conflicts - freshMachine := &mapiv1beta1.Machine{} - if err := cl.Get(ctx, client.ObjectKeyFromObject(machine), freshMachine); err != nil { - return err - } - - if err := updateAWSMachineProviderSpec(freshMachine, updateFunc); err != nil { - return err - } - - return cl.Update(ctx, freshMachine) - }, capiframework.WaitMedium, capiframework.RetryMedium).Should( - MatchError(ContainSubstring(expectedError)), - "Expected AWS providerSpec.%s update to be blocked by VAP", fieldName) -} +func verifyAWSProviderSpecUpdatePrevented(machine *mapiv1beta1.Machine, fieldName string, updateFunc func(*mapiv1beta1.AWSMachineProviderConfig), expectedError string) { + By(fmt.Sprintf("Verifying that updating AWS providerSpec.%s is prevented by VAP", fieldName)) + Eventually(komega.Update(machine, func() { + // Mutate the fresh object fetched by komega.Update + ps := &mapiv1beta1.AWSMachineProviderConfig{} + Expect(json.Unmarshal(machine.Spec.ProviderSpec.Value.Raw, ps)).To(Succeed()) + updateFunc(ps) + b, err := json.Marshal(ps) + Expect(err).ToNot(HaveOccurred()) + machine.Spec.ProviderSpec.Value = &runtime.RawExtension{Raw: b} + }), capiframework.WaitMedium, capiframework.RetryMedium).Should( + MatchError(ContainSubstring(expectedError)), + fmt.Sprintf("Expected AWS providerSpec.%s update to be blocked by VAP", fieldName), + ) +}
242-245: Use %w when wrapping errors.Allows unwrapping with errors.Is/As.
- return fmt.Errorf("failed to marshal modified providerSpec: %v", err) + return fmt.Errorf("failed to marshal modified providerSpec: %w", err)- return nil, fmt.Errorf("failed to unmarshal providerSpec: %v", err) + return nil, fmt.Errorf("failed to unmarshal providerSpec: %w", err)Also applies to: 258-260
110-118: Optional: Add a deletion case for protected labels.To fully exercise “add/modify/delete”, add a test that attempts to delete an existing protected label (ensure it exists first).
130-138: Optional: Add a deletion case for protected annotations.Mirror the label-deletion suggestion for annotations; pre-create the annotation or switch authority to allow creation, then verify deletion is blocked under ClusterAPI.
208-212: Broaden the MachineAPI authority bypass check.Also verify non-protected label/annotation updates succeed when authoritativeAPI=MachineAPI to cover metadata path.
It("should not apply VAP when authoritativeAPI is MachineAPI", func() { verifyVAPNotAppliedForMachineAPIAuthority() })Inside verifyVAPNotAppliedForMachineAPIAuthority:
// Verify we can update spec fields (VAP should not apply) Eventually(komega.Update(testMachine, func() { // Try to update a spec field - this should be allowed since VAP doesn't apply providerIDValue := testProviderID testMachine.Spec.ProviderID = &providerIDValue }), capiframework.WaitMedium, capiframework.RetryMedium).Should(Succeed(), "Expected spec update to succeed when authoritativeAPI is MachineAPI (VAP should not apply)") + + // Also verify non-protected metadata updates are allowed + Eventually(komega.Update(testMachine, func() { + if testMachine.Labels == nil { + testMachine.Labels = map[string]string{} + } + testMachine.Labels["vap-allow-label"] = "ok" + if testMachine.Annotations == nil { + testMachine.Annotations = map[string]string{} + } + testMachine.Annotations["vap-allow-annotation"] = "ok" + }), capiframework.WaitMedium, capiframework.RetryMedium).Should(Succeed())Also applies to: 298-305
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
e2e/machine_migration_vap_tests.go(1 hunks)
🔇 Additional comments (2)
e2e/machine_migration_vap_tests.go (2)
55-70: Nice flake guard: waiting for authoritativeAPI=ClusterAPI before enforcing.This explicit wait should reduce CI flakes and aligns with the VAP match condition. Looks good.
233-249: Confirm TypeMeta is preserved in providerSpec Raw.JSON round-trip can drop kind/apiVersion if empty. Please verify they remain set after mutations to avoid downstream decode issues; if needed, re-set from the pre-mutation object when blank.
Also applies to: 251-264
e2e/machine_migration_vap_tests.go
Outdated
| Eventually(func() (mapiv1beta1.MachineAuthority, error) { | ||
| fresh := &mapiv1beta1.Machine{} | ||
| err := cl.Get(ctx, client.ObjectKeyFromObject(testMAPIMachine), fresh) | ||
| return fresh.Status.AuthoritativeAPI, err | ||
| }, capiframework.WaitLong, capiframework.RetryMedium).Should( | ||
| Equal(mapiv1beta1.MachineAuthorityClusterAPI), | ||
| "VAP requires status.authoritativeAPI=Cluster API before enforcement", | ||
| ) |
There was a problem hiding this comment.
can we change to Eventually(komega.Get(...))
e2e/machine_migration_vap_tests.go
Outdated
| if machine.Spec.ProviderSpec.Value == nil { | ||
| return nil, fmt.Errorf("providerSpec is nil") | ||
| } |
There was a problem hiding this comment.
Expect(machine.Spec.ProviderSpec.Value).ToNot(BeNil())
e2e/machine_migration_vap_tests.go
Outdated
| if err := json.Unmarshal(machine.Spec.ProviderSpec.Value.Raw, providerSpec); err != nil { | ||
| return nil, fmt.Errorf("failed to unmarshal providerSpec: %v", err) | ||
| } |
There was a problem hiding this comment.
Expect(machine.Spec.ProviderSpec.Value.Raw, providerSpec)).To(Succeed())
There was a problem hiding this comment.
there will be multiple changes , let me follow this pattern. Thanks.
removing the part which is covered in other PRs co-author: cursor & coderabbitAI
miyadav
force-pushed
the
2566vaptests_latest
branch
from
b01f005 to
2eb2ed9
Compare
|
@miyadav: This pull request references OCPCLOUD-2566 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target either version "4.21." or "openshift-4.21.", but it targets "openshift-4.20" instead. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
e2e/machine_migration_vap_tests.go (1)
258-277: Consider using Komega for consistency.While the current implementation works,
verifyAWSProviderSpecUpdatePreventeduses directcl.Getandcl.Updatecalls, whereas other helpers (verifyUpdatePrevented,verifyUpdateAllowed) use Komega. For consistency and to follow the pattern established elsewhere in the test suite, consider refactoring to use Komega utilities.The current approach is functional, so this is optional and can be deferred if the refactor adds complexity without clear benefit.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
e2e/machine_migration_vap_tests.go(1 hunks)
|
/test okd-scos-e2e-aws-ovn |
|
@theobarberbany PTAL when time permits , looking to get this in :D . |
|
One change to consider, otherwise LGTM! What's the global timeout on these VAP tests? /lgtm |
openshift-ci
bot
added
lgtm
|
@miyadav: This pull request references OCPCLOUD-2566 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target either version "4.21." or "openshift-4.21.", but it targets "openshift-4.20" instead. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@theobarberbany we are using waitlong at couple of places which is 15min, reducing it to waitMedium ( 3 mins ) , I think that is sufficient for the test. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
e2e/machine_migration_vap_tests.go (1)
258-277: Consider using Komega for consistency.While
verifyUpdatePreventedandverifyUpdateAlloweduse Komega (lines 219, 228), this helper uses directcl.Getandcl.Updatecalls. Although previous reviews suggested Komega throughout, the current pattern may be intentional to handle the error return fromupdateAWSMachineProviderSpec.For consistency and to avoid potential confusion when
updateAWSMachineProviderSpecfails with a non-VAP error, consider refactoring to separate concerns: handle unmarshal/marshal errors withExpect(fail fast on setup errors), then use Komega for the update assertion.Example refactor:
func verifyAWSProviderSpecUpdatePrevented(machine *mapiv1beta1.Machine, fieldName string, updateFunc func(*mapiv1beta1.AWSMachineProviderConfig), expectedError string) { By(fmt.Sprintf("Verifying that updating AWS providerSpec.%s is prevented by VAP", fieldName)) - Eventually(func() error { - // Get fresh copy to avoid conflicts - freshMachine := &mapiv1beta1.Machine{} - if err := cl.Get(ctx, client.ObjectKeyFromObject(machine), freshMachine); err != nil { - return err - } - - if err := updateAWSMachineProviderSpec(freshMachine, updateFunc); err != nil { - return err - } - - return cl.Update(ctx, freshMachine) - }, capiframework.WaitMedium, capiframework.RetryMedium).Should( + Eventually(komega.Update(machine, func() { + Expect(updateAWSMachineProviderSpec(machine, updateFunc)).To(Succeed()) + }), capiframework.WaitMedium, capiframework.RetryMedium).Should( MatchError(ContainSubstring(expectedError)), "Expected AWS providerSpec.%s update to be blocked by VAP", fieldName) }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
e2e/machine_migration_vap_tests.go(1 hunks)
🔇 Additional comments (6)
e2e/machine_migration_vap_tests.go (6)
3-37: LGTM! Constants and imports are well-organized.The imports are appropriate for the test suite, and the constants clearly define test values and expected VAP error messages. The use of
encoding/jsonis correct for providerSpec handling.
39-79: LGTM! Test setup is robust.The test suite properly gates execution on AWS platform and MachineAPIMigration feature flag. The wait for
status.authoritativeAPI(lines 62-68) correctly addresses potential race conditions, and cleanup is properly handled withDeferCleanup.
81-212: LGTM! Comprehensive test coverage.The test contexts provide thorough coverage of VAP enforcement scenarios, including:
- Spec field restrictions (providerID, taints, metadata)
- Protected vs non-protected labels and annotations
- Multiple AWS providerSpec fields
- VAP bypass verification when authoritativeAPI is MachineAPI
Each test is focused and uses the appropriate helper functions.
215-230: LGTM! Helpers use Komega correctly.Both
verifyUpdatePreventedandverifyUpdateAllowedproperly usekomega.Updatefor consistency with the codebase and testing best practices.
232-256: LGTM! ProviderSpec extraction and update helpers are correct.The helpers properly use
encoding/jsonfor marshaling/unmarshaling providerSpec, avoiding the YAML-vs-JSON issues from earlier reviews. The use ofExpectingetAWSProviderSpecFromMachineprovides clear failure messages if providerSpec is missing.
279-306: LGTM! VAP bypass test is well-structured.The test correctly:
- Creates a machine with MachineAPI authority
- Waits for
status.authoritativeAPIto be set (lines 286-292), preventing race conditions- Verifies spec updates are allowed when VAP doesn't apply
- Uses
komega.Updatefor consistencyThe use of
mapiframework.DeleteMachinesfor cleanup (line 296) differs fromcleanupMachineResourcesused in the main test (line 72), but both are valid approaches.
miyadav
force-pushed
the
2566vaptests_latest
branch
from
a5aebc1 to
354042a
Compare
|
@miyadav: This pull request references OCPCLOUD-2566 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target either version "4.21." or "openshift-4.21.", but it targets "openshift-4.20" instead. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
reduce time,changes to follow consistent pattern
miyadav
force-pushed
the
2566vaptests_latest
branch
from
354042a to
ebde40b
Compare
|
/lgtm |
|
@miyadav: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/assign @damdo |
|
/approve |
openshift-ci
bot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: theobarberbany The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
6 participants
@sunzhaohua2 @huali9 PTAL when time permits , I have to omit few cases as error messages were not there for them , they actually get reconciled to old values , example labels of machinetype even after changing.
Also , my apologies the PR - #355 has been opened so long , even I forgot changes so , I took the rebase of latest code and recreated this one.
I will close that one.
Summary by CodeRabbit