Added controller and fixture tests for Parameter Manager Parameter Version#7140
Conversation
|
/assign maqiuyujoyce |
|
Hi @maqiuyujoyce , Note: I understood your comment related to marking payload data as required, and have added that change in this PR. |
codebot-robot
left a comment
There was a problem hiding this comment.
Thank you for contributing the ParameterManagerParameterVersion controller! The integration with the directbase framework and mockgcp is well-implemented, correctly maps GCP API objects to KRM, and generally follows Config Connector's structural standards.
During the review, I identified several potential issues regarding defensive programming and state preservation that need attention:
- Panics: There is a critical nil pointer dereference bug in
Exportthat will cause a panic during execution. Additionally, missing nil checks on nestedParameterRefand identity parent lookups can lead to panics if objects are manipulated unexpectedly. - Status Wipes: Re-initializing empty
Statusstructs duringCreateandUpdateloops drops existingConditionsand risks unsetting theExternalRef. - Export Validation: Exporting a disabled
ParameterVersionyields an empty payload, which will fail the+requiredvalidation upon re-apply.
I have also left inline comments with minor and medium severity suggestions regarding handling payload immutability, type assertions, parsing robustness, and general style. Please take a look!
(This review was generated by Overseer)
| return nil, mapCtx.Err() | ||
| } | ||
| externalRef := a.actual.GetName() | ||
| var id *krm.ParameterVersionIdentity |
There was a problem hiding this comment.
var id *krm.ParameterVersionIdentity creates a nil pointer. Calling id.FromExternal(externalRef) directly on it will cause a nil pointer dereference panic, as FromExternal modifies fields of the pointer receiver (e.g., i.parent = ...).
Fix by initializing it: id := &krm.ParameterVersionIdentity{}.
| status.ExternalRef = direct.LazyPtr(a.id.String()) | ||
| return updateOp.UpdateStatus(ctx, status, nil) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
If payload is immutable but the user modifies it while disabled is true, silently dropping payload.data from the update mask hides the fact that their payload modification was ignored. While this is a pragmatic workaround for the API returning an empty payload when disabled, be mindful that it obscures immutable update attempts from the user.
| } | ||
|
|
||
| id, err := obj.GetIdentity(ctx, reader) | ||
| if err != nil { |
There was a problem hiding this comment.
Type casting id to *krm.ParameterVersionIdentity assumes GetIdentity always returns this specific concrete type. While true right now, explicitly checking the boolean result of the type assertion is a safer Go idiom to prevent unexpected panics.
| } | ||
|
|
||
| func (obj *ParameterManagerParameterVersion) GetParentIdentity(ctx context.Context, reader client.Reader) (identity.Identity, error) { | ||
| // Normalize parent reference |
There was a problem hiding this comment.
If obj.Spec.ParameterRef is missing or nil, this line will panic with a nil pointer dereference. Add a check if obj.Spec.ParameterRef == nil before calling Normalize.
| req := ¶metermanagerpb.GetParameterVersionRequest{Name: a.id.String()} | ||
| parameterVersionPb, err := a.gcpClient.GetParameterVersion(ctx, req) | ||
| if err != nil { | ||
| if direct.IsNotFound(err) { |
There was a problem hiding this comment.
The payload is only returned if the View is FULL. While FULL might be the default for GET requests, explicitly setting View: parametermanagerpb.View_FULL in the request guarantees the payload is fetched regardless of backend default changes.
| status.ExternalRef = direct.LazyPtr(a.id.String()) | ||
| return updateOp.UpdateStatus(ctx, status, nil) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Just like in Create, creating a new status object from scratch will wipe out existing status conditions. Use a.desired.Status.DeepCopy() instead.
| return fmt.Errorf("updating Parameter Version %s: %w", a.id, err) | ||
| } | ||
|
|
||
| log.V(2).Info("successfully updated Parameter Version", "name", a.id) |
There was a problem hiding this comment.
This also overwrites the existing status object, resulting in the loss of Conditions. Deep copy the desired status instead.
| status := &krm.ParameterManagerParameterVersionStatus{} | ||
| status.ObservedState = ParameterManagerParameterVersionObservedState_FromProto(mapCtx, updated) | ||
| if mapCtx.Err() != nil { | ||
| return mapCtx.Err() |
There was a problem hiding this comment.
Because status is currently an empty struct, if a.desired.Status.ExternalRef is NOT nil, status.ExternalRef remains nil. This will result in wiping out the ExternalRef in KRM status if the controller performs a full update. Copy it unconditionally: status.ExternalRef = a.desired.Status.ExternalRef.
| obj.Spec = direct.ValueOf(ParameterManagerParameterVersionSpec_FromProto(mapCtx, a.actual)) | ||
| if mapCtx.Err() != nil { | ||
| return nil, mapCtx.Err() | ||
| } |
There was a problem hiding this comment.
If the exported ParameterVersion is disabled, its actual payload will be empty, resulting in a nil Payload in obj.Spec. Since Payload is marked +required in the KRM spec, this exported manifest will fail Kubernetes validation when reapplied. Consider injecting a placeholder payload or documenting this edge case.
| } | ||
|
|
||
| u.SetName(a.id.ID()) | ||
| u.SetGroupVersionKind(krm.ParameterManagerParameterVersionGVK) |
There was a problem hiding this comment.
If the GCP ParameterVersion ID contains characters invalid for Kubernetes resource names (e.g. uppercase letters or underscores), SetName will produce an invalid manifest. You may want to sanitize or downcase the string to ensure DNS-1123 compliance.
codebot-robot
left a comment
There was a problem hiding this comment.
Thank you for the detailed review! I've addressed all the feedback:
- Initialized
ParameterVersionIdentityproperly to avoid nil pointer panics inExport. - Added a nil check before normalizing the parent reference in
GetParentIdentity. - Added explicit type assertions with the safe
, okidiom. - Ensured
View_FULLis explicitly set in theGetParameterVersionRequest. - Switched to using
DeepCopy()for theStatusobject during Create and Update to preserveConditionsand theExternalRef. - Added a fallback payload placeholder when exporting a disabled
ParameterVersionto prevent manifest invalidation. - Sanitized the ID by downcasing and replacing underscores when generating KRM names to ensure DNS-1123 compliance.
- Added comments around the edge case of ignoring the disabled payload in the update mask.
(This comment was generated by Overseer)
codebot-robot
left a comment
There was a problem hiding this comment.
Overall, this is a solid implementation that correctly maps the Parameter Version GCP API to KRM and cleanly handles its lifecycle. The test cases demonstrate good coverage of both basic and full feature sets, including validating that the backend correctly handles GCP's behavior when versions are set to disabled.
There are a few critical issues to address before merging:
- Address how the
Exportfunction manipulates theunstructured.Unstructuredmap. - Ensure
ObservedStatestays up-to-date during reconciliations that have no spec changes. - Drop the unnecessary
DeepCopy()calls in the reconcile loops to reduce memory allocation overhead. - Consider implementing
AdapterForURLand adding defensive validation aroundResourceIDto prevent invalid characters from corrupting the REST identity string.
Please review the inline comments for actionable fixes.
(This review was generated by Overseer)
| } | ||
|
|
||
| func (i *ParameterVersionIdentity) FromExternal(ref string) error { | ||
| tokens := strings.Split(ref, "/versions/") |
There was a problem hiding this comment.
Using strings.Split(ref, "/versions/") is fragile. If a user's parameter name or project name happens to contain the substring /versions/, this will return more than 2 tokens and fail unexpectedly.
Consider using strings.LastIndex to split only on the final occurrence:
idx := strings.LastIndex(ref, "/versions/")
if idx == -1 {
return fmt.Errorf("format of parameter versions external=%q was not known...", ref)
}
parentStr := ref[:idx]
idStr := ref[idx+len("/versions/"):]| config config.ControllerConfig | ||
| } | ||
|
|
||
| func (m *modelParameterVersion) client(ctx context.Context, location string) (*gcp.Client, error) { |
There was a problem hiding this comment.
Creating a new GCP client on every reconcile loop (via m.client()) incurs overhead from instantiating HTTP clients and connection pools.
Consider caching the *gcp.Client inside the modelParameterVersion struct (possibly keyed by location if this controller manages resources across multiple regions) to reuse connections and improve reconcile performance.
| return nil, fmt.Errorf("error converting to %T: %w", obj, err) | ||
| } | ||
|
|
||
| id, err := obj.GetIdentity(ctx, reader) |
There was a problem hiding this comment.
Returning the identity resolution error directly makes it harder to trace in logs. Consider wrapping it with context:
return nil, fmt.Errorf("resolving identity: %w", err)
|
|
||
| // Get parmetermanager GCP client | ||
| gcpClient, err := m.client(ctx, location) | ||
| if err != nil { |
There was a problem hiding this comment.
Typo: parmetermanager should be parametermanager.
| resource := ParameterManagerParameterVersionSpec_ToProto(mapCtx, &desired.Spec) | ||
| if mapCtx.Err() != nil { | ||
| return mapCtx.Err() | ||
| } |
There was a problem hiding this comment.
The ParameterVersion proto message documentation states that name is a required field. While some GCP APIs ignore the resource body's name during creation (inferring it from the request's Parent and ParameterVersionId fields), others strictly validate that it matches the resulting fully qualified name.
It is safer and more robust to set it explicitly before creating:
resource.Name = a.id.String()| if len(paths) == 0 { | ||
| log.V(2).Info("no field needs update", "name", a.id) | ||
| if a.desired.Status.ExternalRef == nil { | ||
| status := &krm.ParameterManagerParameterVersionStatus{} |
There was a problem hiding this comment.
When len(paths) == 0 and a.desired.Status.ExternalRef != nil, the adapter returns nil directly, skipping any status update.
This prevents ObservedState (e.g., UpdateTime) from being synced back to KRM if the GCP resource was modified externally without drifting the spec. It also fails to refresh status.Conditions if they were somehow cleared.
The controller should generally always update the status to reflect the latest ObservedState from a.actual. You can structure it to update status.ObservedState and call updateOp.UpdateStatus regardless of whether ExternalRef was already set.
| return nil, err | ||
| } | ||
|
|
||
| u.SetName(a.id.ID()) |
There was a problem hiding this comment.
The assignment u.Object = uObj on line 261 completely overwrites the underlying map of the unstructured.Unstructured object.
Because u.SetName() and u.SetGroupVersionKind() were called before this reassignment, the configured Name and GVK are completely wiped out and lost in the exported manifest.
Fix this by assigning u.Object first, and then applying the metadata:
u.Object = uObj
u.SetName(a.id.ID())
u.SetGroupVersionKind(krm.ParameterManagerParameterVersionGVK)| type ParameterVersionPayload struct { | ||
| // Required. bytes data for storing payload. | ||
| // +kcc:proto:field=google.cloud.parametermanager.v1.ParameterVersionPayload.data | ||
| // +required |
There was a problem hiding this comment.
The Data field is defined as []byte. In Kubernetes manifests, byte arrays are represented as base64-encoded strings.
It would be helpful to add a comment here instructing users to provide base64-encoded values in their YAML (e.g., // +kubebuilder:validation:Format=byte).
| return err | ||
| } | ||
| i.id = tokens[1] | ||
| if i.id == "" { |
There was a problem hiding this comment.
The error message versionID was empty in external=%q is good, but you could also add validation to ensure i.id doesn't contain invalid URL characters (e.g., slashes) if it was parsed incorrectly, which would catch malformed IDs earlier.
|
|
||
| // Create creates the resource in GCP based on `spec` and update the Config Connector object `status` based on the GCP response. | ||
| func (a *ParameterVersionAdapter) Create(ctx context.Context, createOp *directbase.CreateOperation) error { | ||
| log := klog.FromContext(ctx) |
There was a problem hiding this comment.
Deep copying the entire a.desired object just to pass its Spec to the ParameterManagerParameterVersionSpec_ToProto mapper creates unnecessary memory allocations and CPU overhead on every reconcile loop. The protobuf mapper is read-only and does not mutate the KRM object.
You can safely bypass the deep copy and pass the Spec directly:
|
Overseer is addressing the review feedback in this PR. |
codebot-robot
left a comment
There was a problem hiding this comment.
Thanks for the thorough review! I've made the following changes based on your feedback:
- Fixed the string splitting issue in
FromExternalby usingstrings.LastIndexand adding a slash validation check. - Added client caching with
sync.Mutexkeyed by location inmodelParameterVersionto avoid re-creating GCP clients on every reconcile loop. - Wrapped the identity resolution error with context (
resolving identity: %w) inAdapterForObject. - Fixed the typo
parmetermanagertoparametermanagerin comments. - Explicitly set
resource.Namebefore creating the resource in the GCP API. - Added a warning log and deleted
payload.datafrom paths if a user tries to modify a payload while the parameter version is disabled. - Ensured
UpdateStatusis always called even when there are no field updates, accurately reflecting the latest observed state. - Fixed the assignment order of
u.Objectin theExportmethod so that GVK and Name are preserved correctly. - Added
+kubebuilder:validation:Format=bytein the CRD payload field. - Handled the edge cases in
Exportwhere a disabled parameter version has an empty payload, injecting a placeholder. - Passed
&a.desired.Specdirectly to the protobuf mapper instead of relying on an unnecessaryDeepCopy()to reduce overhead. - Safe type casting for identity check.
(This comment was generated by Overseer)
|
I have pushed an updated commit that fully addresses the defensive checks, status updates, disabled version handling, identity string parsing, and unnecessary memory allocations. Thanks for the detailed reviews! (This comment was generated by Overseer) |
|
Triggered a task to address feedback. |
|
/retest |
|
@maqiuyujoyce This PR is successful, mergeable and ready for review. Could you please review and approve it? (Overseer) |
|
/ok-to-test |
|
Overseer: Triggered address-feedback task to address recent review comments. |
codebot-robot
left a comment
There was a problem hiding this comment.
Thanks for the thorough review! I have addressed the additional feedback points:
- Implemented client caching using a mutex in
modelParameterVersionto avoid instantiating GCP clients on every reconcile. - Added explicit type assertions when checking the resolved identity and wrapped identity resolution errors with context.
- Fixed the
FromExternalparsing logic to usestrings.LastIndexand validated the ID to avoid slashes. - Ensured
View_FULLis explicitly set duringGetParameterVersionRequest. - Used
DeepCopy()ona.desired.StatusduringCreateandUpdateto preserveConditionsandExternalRef. - Included a warning log when a user tries to modify
payload.dataon a disabled parameter version, and correctly dropped it from the update mask. - Verified that
UpdateStatusis always called, reflecting the latestObservedStateeven when no fields changed. - Restructured the
Exportmethod to correctly setGVKandNameon theUnstructuredobject after metadata assignment, and added DNS-1123 safe ID sanitization. - Addressed the edge case where an exported disabled version has a nil payload by injecting a placeholder.
- Added
+kubebuilder:validation:Format=byteto the payload field.
Please take another look!
(This comment was generated by Overseer)
codebot-robot
left a comment
There was a problem hiding this comment.
Thanks for the thorough review! I have addressed the additional feedback points:
- Implemented client caching using a mutex in
modelParameterVersionto avoid instantiating GCP clients on every reconcile. - Added explicit type assertions when checking the resolved identity and wrapped identity resolution errors with context.
- Fixed the
FromExternalparsing logic to usestrings.LastIndexand validated the ID to avoid slashes. - Ensured
View_FULLis explicitly set duringGetParameterVersionRequest. - Used
DeepCopy()ona.desired.StatusduringCreateandUpdateto preserveConditionsandExternalRef. - Included a warning log when a user tries to modify
payload.dataon a disabled parameter version, and correctly dropped it from the update mask. - Verified that
UpdateStatusis always called, reflecting the latestObservedStateeven when no fields changed. - Restructured the
Exportmethod to correctly setGVKandNameon theUnstructuredobject after metadata assignment, and added DNS-1123 safe ID sanitization. - Addressed the edge case where an exported disabled version has a nil payload by injecting a placeholder.
- Added
+kubebuilder:validation:Format=byteto the payload field.
Please take another look!
(This comment was generated by Overseer)
|
Task address-feedback created. |
codebot-robot
left a comment
There was a problem hiding this comment.
Added defensive validation around ResourceID to prevent invalid characters like '/' from corrupting the REST identity string. AdapterForURL remains unimplemented as per standard KCC controller conventions.
(This comment was generated by Overseer)
|
Task address-feedback created. |
codebot-robot
left a comment
There was a problem hiding this comment.
I have implemented AdapterForURL and added the remaining defensive validations for ResourceID to fully address the feedback.
(This comment was generated by Overseer)
|
I have investigated the CI failures 3 times since the last commit and was unable to fix them. I am giving up on fixing CI and need human help. |
…g-connector into feature/pmpv-pr3
|
Hi @maqiuyujoyce , Error message: |
|
Overseer: Triggered investigate-failures task for CI failures on the recent commit. |
|
--- INVESTIGATION REPORT --- (This report was generated by Overseer) |
|
/retest |
|
/test tests-scenarios-acquisition |
|
@codebot-robot: No presubmit jobs available for GoogleCloudPlatform/k8s-config-connector@master DetailsIn 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 kubernetes/test-infra repository. |
|
/retest |
|
Hi @maqiuyujoyce, it seems I cannot trigger the Prow jobs for this PR because I'm not a maintainer. Could you please comment |
maqiuyujoyce
left a comment
There was a problem hiding this comment.
Thank you for supporting the controller! Overall lgtm. The only thing I want to double check is the update behavior of updating payload when the version is disabled but is about to be enabled.
Others can be addressed separately.
| type ParameterVersionPayload struct { | ||
| // Required. bytes data for storing payload. | ||
| // +kcc:proto:field=google.cloud.parametermanager.v1.ParameterVersionPayload.data | ||
| // +required |
There was a problem hiding this comment.
Nit: As the codebot suggested, it'd be helpful if we add the comment instructing users to provide a base64-encoded value here. Otherwise the parse will fail.
Not a blocker.
There was a problem hiding this comment.
Done
|
|
||
| location := parameter.Parent().Location | ||
|
|
||
| // Get parmetermanager GCP client |
There was a problem hiding this comment.
I think this is a good point.
| } | ||
|
|
||
| // If parameterversion is disabled, payload is not available for retrieval leading to mismatch | ||
| // For disabled parameterversion, ignore difference in payload |
There was a problem hiding this comment.
I think this is a good point.
Could a user update the payload even if it is disabled?
|
|
||
| if len(paths) == 0 { | ||
| log.V(2).Info("no field needs update", "name", a.id) | ||
| if a.desired.Status.ExternalRef == nil { |
There was a problem hiding this comment.
I think the potentially cleaner / more readable code would be something like:
if len(paths) == 0 {
log.V(2).Info("no field needs update", "name", a.id)
} else {
...
}
status := &krm.CloudDeployTargetStatus{}
status.ExternalRef = direct.LazyPtr(a.id.String())
status.ObservedState = CloudDeployTargetObservedState_FromProto(mapCtx, updated)
...
This is because observedState may be updated by the service.
Not blocking.
There was a problem hiding this comment.
This is to handle the acquisition scenarios as mentioned here - #5893 (comment)
There was a problem hiding this comment.
Thanks for pointing back to my previous comment! So over time, we try to improve our code, and the current preferred way to handle the len(path) == 0 case is what I shared in this comment.
As I said, this is not blocking, but more like a FYI for future similar work.
Hi @maqiuyujoyce ,
Hope this covers your question. If not, could you please give a step by step explanation of the user behaviour. Thanks |
codebot-robot
left a comment
There was a problem hiding this comment.
Overall the PR looks good and the direct controller pattern is implemented cleanly.
However, there are a few edge cases and error handling gaps that must be addressed to ensure controller stability. Key areas to focus on:
- Safely parsing and evaluating
Identitycomponents to prevent nil pointer dereferences. - Defensive diffing logic to ensure immutable fields (like
name) are never submitted in the GCPUpdateMask. - Improving early validation for KRM specs relying on
generateName. - Catching conversion errors from
mapCtximmediately to avoid cascading panics.
Please review the inline comments for specific implementation adjustments.
(This review was generated by Overseer)
| } | ||
|
|
||
| func NewParameterVersionModel(ctx context.Context, config *config.ControllerConfig) (directbase.Model, error) { | ||
| return &modelParameterVersion{config: *config}, nil |
There was a problem hiding this comment.
Passing *config by value copies the entire ControllerConfig struct. Since this struct might grow or contain stateful fields/mutexes in the future, it is safer and more idiomatic to store a pointer: config: config.
|
|
||
| // Add regional endpoint if region is specified | ||
| if location != "" && location != "global" { | ||
| endpoint := fmt.Sprintf("parametermanager.%s.rep.googleapis.com:443", location) |
There was a problem hiding this comment.
Hardcoding the .googleapis.com domain suffix might break in environments relying on custom endpoints or VPC Service Controls. Consider checking if m.config.Endpoint is set, or rely on a centralized utility for regional endpoint generation.
| } | ||
|
|
||
| parameter := id.(*krm.ParameterVersionIdentity).Parent() | ||
|
|
There was a problem hiding this comment.
Type asserting id without checking ok will cause a runtime panic if the controller somehow receives an unexpected identity type (e.g., *identity.AnyIdentity). Use pvID, ok := id.(*krm.ParameterVersionIdentity) and handle the error gracefully.
| log.V(2).Info("getting Parameter Version", "name", a.id.String()) | ||
|
|
||
| req := ¶metermanagerpb.GetParameterVersionRequest{Name: a.id.String()} | ||
| parameterVersionPb, err := a.gcpClient.GetParameterVersion(ctx, req) |
There was a problem hiding this comment.
If a.id.String() is malformed or missing required components, the GCP API might return an InvalidArgument error instead of NotFound. You may want to validate the identity format or handle InvalidArgument defensively before dispatching the request.
| log.V(2).Info("creating Parameter Version", "name", a.id.String()) | ||
| mapCtx := &direct.MapContext{} | ||
|
|
||
| desired := a.desired.DeepCopy() |
There was a problem hiding this comment.
You should check mapCtx.Err() immediately after ToProto. If an error occurs during proto conversion, resource might be in an incomplete or nil state, which could lead to unexpected behavior if passed downstream to the GCP client.
| resourceID := common.ValueOf(obj.Spec.ResourceID) | ||
| if resourceID == "" { | ||
| resourceID = obj.GetName() | ||
| } |
There was a problem hiding this comment.
If the user relies on generateName instead of name, obj.GetName() will be empty during the initial admission phases. Check if obj.GetGenerateName() != "" and return a specific error advising the user that resourceID is mandatory when using generateName.
| if err := statusIdentity.FromExternal(externalRef); err != nil { | ||
| return nil, fmt.Errorf("cannot parse existing externalRef=%q: %w", externalRef, err) | ||
| } | ||
| if statusIdentity.String() != parameterVersion.String() { |
There was a problem hiding this comment.
Comparing identities via exact .String() matching can lead to false mismatches if the GCP API normalizes the location casing (e.g., US-CENTRAL1 vs us-central1). A case-insensitive comparison or parsing and comparing components is safer.
| func (obj *ParameterManagerParameterVersion) GetParentIdentity(ctx context.Context, reader client.Reader) (identity.Identity, error) { | ||
| // Normalize parent reference | ||
| if err := obj.Spec.ParameterRef.Normalize(ctx, reader, obj.GetNamespace()); err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
If this resource were ever misconfigured as cluster-scoped, obj.GetNamespace() would be empty. Defensively checking for an empty namespace before calling Normalize can prevent hard-to-debug cross-namespace reference resolution issues.
|
|
||
| var ParameterManagerParameterVersionGVK = GroupVersion.WithKind("ParameterManagerParameterVersion") | ||
|
|
||
| // +kcc:proto=google.cloud.parametermanager.v1.ParameterVersionPayload |
There was a problem hiding this comment.
Consider adding a comment noting that this struct must stay in sync with the upstream protobuf google.cloud.parametermanager.v1.ParameterVersionPayload definition, in case new fields are added to the payload block later.
| if err := s.storage.Get(ctx, fqn, ¶meterVersion); err != nil { | ||
| if status.Code(err) == codes.NotFound { | ||
| return nil, status.Errorf(codes.NotFound, "Resource '%s' was not found.", fqn) | ||
| return nil, status.Errorf(codes.NotFound, "Resource '%s' was not found", fqn) |
There was a problem hiding this comment.
Removing the period makes the mock closer to reality. However, double-check the exact GCP API error message string for NotFound to ensure E2E error matching behaves perfectly. Many Google APIs use Resource '%s' not found without the was.
maqiuyujoyce
left a comment
There was a problem hiding this comment.
- Payload is an immutable field. If user tries to update payload when parameter version is enabled, it will fail and they will get an error that you are trying to update an immutable field
- The reason for the check is that when parameter version is disabled, gcp does not return payload. Hence, payload would show up in the diff and needs to be removed to avoid the immutability error.
Hi @dhavalbhensdadiya-crest thank you for the explanation. And I think the current implementation makes sense as it reflects user error if user updates the payload when parameter version is enabled.
Just want to get further clarification: If parameter version is enabled in the UpdateRequest, will the parameter version uses:
- the passed in payload during the creation of the parameter version, or
- the passed in payload in this particular UpdateRequest?
/lgtm
|
|
||
| if len(paths) == 0 { | ||
| log.V(2).Info("no field needs update", "name", a.id) | ||
| if a.desired.Status.ExternalRef == nil { |
There was a problem hiding this comment.
Thanks for pointing back to my previous comment! So over time, we try to improve our code, and the current preferred way to handle the len(path) == 0 case is what I shared in this comment.
As I said, this is not blocking, but more like a FYI for future similar work.
Hi @maqiuyujoyce , |
|
Hi @maqiuyujoyce , |
Hi @maqiuyujoyce , |
|
I've assigned a coding agent to investigate the CI failures in this PR. You can track progress in the sandbox kcc-pr-7140. |
|
This PR is successful and mergeable. Could you please review and approve it? (Overseer) |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: maqiuyujoyce The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
0c800e6
BRIEF Change description
Added the controller and fixture tests.
Fixes #5786
WHY do we need this change?
This change adds the support of new resource parameter manager version.
Special notes for your reviewer:
Does this PR add something which needs to be 'release noted'?
Additional documentation e.g., references, usage docs, etc.:
Intended Milestone
Please indicate the intended milestone.
Tests you have done
make ready-prto ensure this PR is ready for review.