-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add endpoint package #1
Conversation
e7b3d66
to
c14969f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
What do you think of moving
utils/
tointernal/utils/
? Do we want the utils package to be a part of the library's API? -
I'm not going to hold up the PR for it, but we may want move to envtest instead of client/fake for the tests.
// IsHealthy returns whether or not all Kube resources used by endpoint are healthy | ||
IsHealthy(ctx context.Context, c client.Client) (bool, error) | ||
// MarkForCleanup | ||
MarkForCleanup(ctx context.Context, c client.Client, key, value string) error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume they will pass in and remember the k/v that gets applied to the underlying objects, but how will they do the actual cleanup? Are they expected to do the DeleteAllOf
on the types that are returned by a call to APIsToWatch
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refactored, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refactored, thanks!
^ that was in response to a different comment.
But yes, I imagine APIsToWatch feeding the cleanup APIs as well. Open to renaming the function as appropriate.
aa9d578
to
32aa885
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/LGTM
@shawn-hurley: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Signed-off-by: Alay Patel <alay1431@gmail.com>
Signed-off-by: Alay Patel <alay1431@gmail.com>
Co-authored-by: John Strunk <jstrunk@redhat.com> Signed-off-by: Alay Patel <alay1431@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Alay!
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alaypatel07, JohnStrunk, shawn-hurley 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 |
Describe what this PR does
This PR adds an endpoint package with the interface methods and
two implementations. One is Route implementation and the other is LoadBalancer.
Is there anything that requires special attention?
This also attempts to solve two distinct requirements for reconcilers:
New
methodsRelated issues:
None