Update hander/enqueue_annotation.go#2
Merged
varshaprasad96 merged 4 commits intooperator-framework:mainfrom Jul 23, 2020
Merged
Update hander/enqueue_annotation.go#2varshaprasad96 merged 4 commits intooperator-framework:mainfrom
varshaprasad96 merged 4 commits intooperator-framework:mainfrom
Conversation
estroz
reviewed
Jul 21, 2020
| // SetWatchOwnerAnnotation is a helper method to add watch annotation-based with the owner NamespacedName and | ||
| // schema.GroupKind to the object provided. This allows you to declare the watch annotations of an owner to an object. | ||
| // If a annotation to the same object already exists, it'll be overwritten with the newly provided version. | ||
| func SetOwnerAnnotation(owner, object metav1.Object, ownerGK schema.GroupKind) error { |
Contributor
There was a problem hiding this comment.
See that it is a helper. so, we need to update the places in SDK that are using the annotation.
Contributor
There was a problem hiding this comment.
This PR kubernetes-sigs/controller-runtime#892 received a lot of contributions from the devs in the controller-runtime. So, I think it is in good shape. What is missing to do here;
- Cleanup the go mod
- Update the places across the project to use the helper
- We need to check the CI. (lint to ensure that all is fine, test to check if these tests are ok, ansible and helm as well to check that nothing broke after that and all is properly updated)
varshaprasad96
force-pushed
the
annotations/watch-resources
branch
from
2726c3e to
4ba30c9
Compare
joelanford
requested changes
Jul 21, 2020
Member
joelanford
left a comment
joelanford
left a comment
There was a problem hiding this comment.
Unit tests pass for me once I remove the unnecessary dependency on running envtest. However, given all the nits and discrepancies I found, I'm a little concerned with the validity of the tests.
Let's make sure to double/triple check everything before we merge.
92 tasks
openshift-ci-robot
added
the
needs-rebase
varshaprasad96
force-pushed
the
annotations/watch-resources
branch
from
7b3c512 to
2a45e4d
Compare
This commit updates the implementation in enqueue_annotation, to watch dependent resources based on the presence of an annotation on them. Co-Authored-by: Camila Macedo <cmacedo@redhat.com>
varshaprasad96
force-pushed
the
annotations/watch-resources
branch
from
2a45e4d to
f03d3a7
Compare
openshift-ci-robot
removed
the
needs-rebase
varshaprasad96
force-pushed
the
annotations/watch-resources
branch
3 times, most recently
from
5835cd5 to
c15d42b
Compare
Member
Author
|
Addressed review comments, and made changes to the PR. |
estroz
reviewed
Jul 22, 2020
varshaprasad96
force-pushed
the
annotations/watch-resources
branch
from
c15d42b to
2d6fca2
Compare
Pull Request Test Coverage Report for Build 180371929
💛 - Coveralls |
estroz
suggested changes
Jul 22, 2020
Member
estroz
left a comment
estroz
left a comment
There was a problem hiding this comment.
Mostly nits and grammar, looks great otherwise.
varshaprasad96
force-pushed
the
annotations/watch-resources
branch
from
2d6fca2 to
b9bbc70
Compare
varshaprasad96
force-pushed
the
annotations/watch-resources
branch
from
b9bbc70 to
9d29dbd
Compare
joelanford
reviewed
Jul 23, 2020
varshaprasad96
force-pushed
the
annotations/watch-resources
branch
2 times, most recently
from
ce9b384 to
5be29f3
Compare
varshaprasad96
force-pushed
the
annotations/watch-resources
branch
from
5be29f3 to
bc589e5
Compare
|
New changes are detected. LGTM label has been removed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This commit updates the implementation in enqueue_annotation, to
watch dependent resources based on the presence of an annotation.
Motivation: (Taken from PR #892 of Controller Runtime)
While owner references are the idiomatic way to manage dependent resources, they don't solve all use cases. Owner references have restrictions that limit which objects can own other objects.
An annotation-based watch handler does not have these restrictions. However, they don't provide the same feature set either.
The watch handler can be used to trigger reconciliation of a parent resource based on changes to dependent resources that include an annotation. The purpose of this handler is to support cross-scope ownership relationships that are not supported by native owner references.
For example, owner references are used for garbage collection. If a resource's owner is deleted, the resource will also be deleted. Because of this, annotation references need to be paired with a finalizer so that the operator can manually garbage collect.
Co-Authored-by: Camila Macedo cmacedo@redhat.com