feat: support setting maintenanceVersion on CREATE #4857
Conversation
|
/hold will wait on #4838 to merge first. |
401f26c to
4544723
Compare
17b2212 to
3eab59b
Compare
8809383 to
f36b5fc
Compare
|
/hold cancel |
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
genai: Improve the fidelity of this mock to mimick the behavior of the real server on CREATE when trying to pass in the maintenenaceVersion pasted log results Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
f36b5fc to
c77d6f6
Compare
|
qq: if the create operation is successful but the patch operation failed, that means the resource exist but without the |
There was a problem hiding this comment.
re
qq: if the create operation is successful but the patch operation failed, that means the resource exist but without the maintenanceVersion?
yes that might be the case but the GCP API server will have defaulted the maintenenceVersion already.
There was a problem hiding this comment.
do we let the user know that they have failed to set the value or we will keep trying to update the value during reconciliation?
There was a problem hiding this comment.
both as I understand it! we will also attempt that to re-reconcile via UPDATE next time around
|
/lgtm |
| name: sqlinstance-postgres-minimal-direct-${uniqueId} | ||
| annotations: | ||
| alpha.cnrm.cloud.google.com/reconciler: "direct" | ||
| spec: |
There was a problem hiding this comment.
labels:
configsync.gke.io: "true"
| name: sqlinstance-postgres-minimal-direct-${uniqueId} | ||
| annotations: | ||
| alpha.cnrm.cloud.google.com/reconciler: "direct" | ||
| spec: |
There was a problem hiding this comment.
labels:
configsync.gke.io: "true"
| } | ||
|
|
||
| if err := a.insertInstance(ctx, u, log); err != nil { | ||
| return err |
There was a problem hiding this comment.
Might be nice to wrap the error to bring some additional context. Eg "failed to perform initial insert: %w"
| return fmt.Errorf("patching SQLInstance %s maintenanceVersion failed: %w", a.resourceID, err) | ||
| } | ||
| if err := a.pollForLROCompletion(ctx, op, "maintenanceVersion patch"); err != nil { | ||
| return err |
There was a problem hiding this comment.
Might be nice to wrap the error to bring some additional context. Eg "failed waiting for maintenance version patch: %w"
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: yuwenma 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 |
|
I missed the other reviewers' comments while LGTM the PR. @acpana could you resolve the comments in follow-up PRs? |
Follow up on #4838 (comment)
Does this PR add something which needs to be 'release noted'?