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

chore: Add context to ACK isUpToDate and return diff #1830

Merged

Conversation

MisterMX
Copy link
Collaborator

@MisterMX MisterMX commented Aug 4, 2023

Description of your changes

Fixes #1828
Depends on aws-controllers-k8s/code-generator#458

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

Most of the code remains untouched. Only function parameters were added and context.TODO was replaced with the real context. In some cases the controllers and tests needed to bed changed since their weren't using the ...WithContext APIs. In some other cases cmp.Equal was replaced with diff := cmp.Diff(...); diff != "" to keep the diff and return it later.

@MisterMX MisterMX requested a review from haarchri August 4, 2023 13:12
@MisterMX MisterMX force-pushed the chore/isuptodate-context-diff branch 4 times, most recently from baeefce to 8fc2fbb Compare August 9, 2023 11:03
@chlunde
Copy link
Collaborator

chlunde commented Aug 9, 2023

I'm not super fan about the name utils, ref. https://github.com/golang/go/wiki/CodeReviewComments#package-names - if you have an alternative that would be nice.

And as you say, we should wait with this until merged upstream to prevent depending on a unmerged PR.

maybe in a followup, it would be nice if cases like this returned a hint like "spec.forProvider.pitStatus" instead of the empty string?

	if !isPitrUpToDate(cr, pitrStatusBool) {
		return false, "", nil

Signed-off-by: Maximilian Blatt (external expert on behalf of DB Netz) <[email protected]>
@MisterMX MisterMX force-pushed the chore/isuptodate-context-diff branch from 8fc2fbb to c230c38 Compare August 28, 2023 11:05
@MisterMX MisterMX merged commit 4959099 into crossplane-contrib:master Aug 28, 2023
@MisterMX MisterMX deleted the chore/isuptodate-context-diff branch October 19, 2023 08:49
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.

Add context.Context to ack-generated isUpToDate functions
2 participants