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.

Sign up for GitHub

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
Closed

[chore][extension/observer] Assign exported function observer.NewEndpointsWatcher to a variable and pass checkapi #29321

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions cmd/checkapi/allowlist.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,2 @@
connector/servicegraphconnector
extension/observer
processor/servicegraphprocessor
processor/servicegraphprocessor
2 changes: 1 addition & 1 deletion extension/observer/endpointswatcher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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.

return &EndpointsWatcher{
EndpointsLister: endpointsLister,
RefreshInterval: refreshInterval,
Expand Down