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][extension/observer] Assign exported function observer.NewEndpointsWatcher to a variable and pass checkapi #29321

Closed
wants to merge 1 commit into from

Conversation

sakulali
Copy link
Contributor

Description:
Assign exported function observer.NewEndpointsWatcher to a variable and pass checkapi.

Link to tracking Issue:
#26304

Testing:
go run cmd/checkapi/main.go .
go test for observer
go test for dockerobserver
go test for ecsobserver
go test for ecstaskobserver
go test for hostobserver
go test for k8sobserver

Documentation:

…pointsWatcher` to a variable and pass checkapi

Signed-off-by: sakulali <sakulali@126.com>
@@ -31,7 +31,7 @@ type EndpointsWatcher struct {
logger *zap.Logger
}

func NewEndpointsWatcher(endpointsLister EndpointsLister, refreshInterval time.Duration, logger *zap.Logger) *EndpointsWatcher {
var NewEndpointsWatcher = func(endpointsLister EndpointsLister, refreshInterval time.Duration, logger *zap.Logger) *EndpointsWatcher {
Copy link
Contributor

Choose a reason for hiding this comment

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

That's cheating :) there is no functional difference here. Since this is acting as a library, maybe we can discuss moving this to internal.

Copy link
Contributor Author

@sakulali sakulali Nov 30, 2023

Choose a reason for hiding this comment

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

Thanks @atoulme! This sounds more reasonable since NewEndpointsWatcher is being used by multiple observers, reopen issue 29083 here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should i close or draft this PR since we maybe need to move this to internal?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd close the PR. Thanks for the work on this, it's possible that moving the code to internal is rejected on the basis that this package is considered as a library, but not sure.

Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Dec 14, 2023
Copy link
Contributor

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Dec 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants