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

Update observed gen on no-update #697

Merged
merged 5 commits into from
Nov 24, 2021
Merged

Conversation

csviri
Copy link
Collaborator

@csviri csviri commented Nov 23, 2021

No description provided.

}

private void updateStatusObservedGenerationIfRequired(R resource) {
// todo: change this to check for HasStatus (or similar) when
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you remove this comment?

Copy link
Collaborator Author

@csviri csviri Nov 23, 2021

Choose a reason for hiding this comment

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

don't like comments in general, but actually though about it and the HasStatus resources would make sense to us only if the standard kuberenetes resources would also implement observed generation aware too. What is quite a big change again in fabric8 client. Also not sure if makes sense for a controller which handles the standard kubernetes objects side by side with it's standard controller to manage observed generations. So IMHO we should support this only for custom resources. But wanted to discuss this with the fabtic8 team too, maybe today.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense. That said, the todo comment shows up in IDEA so it's interesting to keep these until we're sure we've addressed what they're referring to, generally.

Copy link
Collaborator

@metacosm metacosm left a comment

Choose a reason for hiding this comment

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

LGTM

@csviri csviri merged commit 609682e into v2 Nov 24, 2021
@csviri csviri deleted the update-observed-gen-on-noupdate branch November 24, 2021 08:29
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.

Automatic observed generation handling and UpdateControl.noUpdate() behavior
2 participants