Skip to content

Conversation

acpana
Copy link
Collaborator

@acpana acpana commented Jul 23, 2025

Change description

Fixes #4811

Special notes for your reviewer:

Does this PR add something which needs to be 'release noted'?

fix: set status on acquisition for SQLInstance controller
  • Reviewer reviewed release note.

Additional documentation e.g., references, usage docs, etc.:


Intended Milestone

Please indicate the intended milestone.

  • Reviewer tagged PR with the actual milestone.

Comment on lines +18 to +24
status:
conditions:
- lastTransitionTime: "1970-01-01T00:00:00Z"
message: The resource is up to date
reason: UpToDate
status: "True"
type: Ready
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

compare this with object 01, notice the lack of additional fields

@acpana
Copy link
Collaborator Author

acpana commented Jul 23, 2025

I think the CI is broken at HEAD!

acpana added 2 commits July 28, 2025 19:45
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
@acpana acpana force-pushed the acpana/sqlinstance-fields-status branch from 5026b03 to 7aa6a6d Compare July 28, 2025 19:46
settings:
tier: db-f1-micro
---
TEST: ABANDON
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we use ABANDON-AND-REACQUIRE directly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the test needs a resourceID and I didn't want to introduce that to this yaml.

return err
}

instanceForStatus := a.actual
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe use updated := a.actual so that we don't need this additional variable instanceForStatus.

for example:

updated := a.actual
if xxx {
...
updated, err = a.sqlInstancesClient.Get(a.projectID, a.resourceID).Context(ctx).Do()
...
}
status, err := SQLInstanceStatusGCPToKRM(updated)

@gemmahou
Copy link
Collaborator

/lgtm

Copy link
Collaborator

@yuwenma yuwenma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

One nit. Non blocker.

Comment on lines +6 to +7
annotations:
cnrm.cloud.google.com/deletion-policy: abandon
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: ABANDON sets the "deletion-policy: abandon" already.

setAnnotation(h, obj, "cnrm.cloud.google.com/deletion-policy", "abandon")

Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-prow google-oss-prow bot merged commit 6b61135 into master Jul 30, 2025
96 of 98 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SQLInstance status incomplete when adopting existing resource
3 participants