Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(ack): Do not call isUpToDate if resource is deleted #1924

Conversation

MisterMX
Copy link
Collaborator

Description of your changes

Modifies the ACK controller template to only call isUpToDate if not meta.WasDeleted(cr). This may safe some time and unnecessary AWS calls.

Only affects controllers that are generated with ACK.

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

n.a.

Only affects controllers that are generated with ACK.

Signed-off-by: Maximilian Blatt (external expert on behalf of DB Netz) <maximilian.blatt-extern@deutschebahn.com>
@MisterMX MisterMX requested a review from chlunde October 20, 2023 11:06
@MisterMX MisterMX changed the title feat(ack): Do not call isUpToDate is resource is deleted feat(ack): Do not call isUpToDate if resource is deleted Oct 20, 2023
@MisterMX MisterMX force-pushed the feat/ack-no-isuptodate-if-deleted branch from f0b4f80 to 77a8032 Compare October 20, 2023 11:48
@chlunde
Copy link
Collaborator

chlunde commented Oct 24, 2023

have you done any manual testing? I think this looks sane, but it is a type of change that could have unintended consequences

@MisterMX
Copy link
Collaborator Author

MisterMX commented Nov 9, 2023

have you done any manual testing? I think this looks sane, but it is a type of change that could have unintended consequences

I only tested this once with rds.DBInstance to check whether the implementation works fine. I did not test the other resources. However, I looked through all isUpToDate functions to see if there is something umcommon (like anything besides read operations) but that does not seem to be the case.

@MisterMX MisterMX merged commit f816fec into crossplane-contrib:master Nov 9, 2023
9 checks passed
@MisterMX MisterMX deleted the feat/ack-no-isuptodate-if-deleted branch November 9, 2023 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants