Skip to content

Move k8s_watcher.go to pkg/k8s/watchers#9434

Merged
aanm merged 3 commits into
masterfrom
pr/refactor-k8s-watcher
Oct 21, 2019
Merged

Move k8s_watcher.go to pkg/k8s/watchers#9434
aanm merged 3 commits into
masterfrom
pr/refactor-k8s-watcher

Conversation

@aanm
Copy link
Copy Markdown
Member

@aanm aanm commented Oct 17, 2019 •
edited
Loading

NOTE: ⚠️ This PR depends on #9400 ⚠️

Sorry but I couldn't split the first commit in smaller commits.

This change is Reviewable

@aanm aanm added pending-review kind/cleanup This includes no functional changes. labels Oct 17, 2019
@aanm aanm requested a review from a team October 17, 2019 19:47
@aanm aanm requested review from a team as code owners October 17, 2019 19:47
@aanm aanm requested review from a team October 17, 2019 19:47
houndci-bot
houndci-bot reviewed Oct 17, 2019
View reviewed changes
Comment thread pkg/ipam/ipam.go Outdated
houndci-bot
houndci-bot reviewed Oct 17, 2019
View reviewed changes
Comment thread pkg/ipam/ipam.go Outdated
houndci-bot
houndci-bot reviewed Oct 17, 2019
View reviewed changes
Comment thread pkg/k8s/watchers/watcher.go Outdated
Copy link
Copy Markdown

@houndci-bot houndci-bot Oct 17, 2019

Choose a reason for hiding this comment

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

exported method K8sWatcher.RunK8sServiceHandler should have comment or be unexported

Comment thread pkg/k8s/watchers/watcher.go Outdated
Copy link
Copy Markdown

@houndci-bot houndci-bot Oct 17, 2019

Choose a reason for hiding this comment

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

exported method K8sWatcher.GetAPIGroups should have comment or be unexported

Comment thread pkg/k8s/watchers/watcher.go Outdated
Copy link
Copy Markdown

@houndci-bot houndci-bot Oct 17, 2019

Choose a reason for hiding this comment

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

exported function NewK8sWatcher should have comment or be unexported

Comment thread pkg/k8s/watchers/watcher.go Outdated
Copy link
Copy Markdown

@houndci-bot houndci-bot Oct 17, 2019

Choose a reason for hiding this comment

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

exported type K8sWatcher should have comment or be unexported

Comment thread pkg/k8s/watchers/watcher.go Outdated
Copy link
Copy Markdown

@houndci-bot houndci-bot Oct 17, 2019

Choose a reason for hiding this comment

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

exported const K8sAPIGroupServiceV1Core should have comment (or a comment on this block) or be unexported

@aanm aanm force-pushed the pr/refactor-k8s-watcher branch from d180e84 to 4a419ea Compare October 17, 2019 20:17
ianvernon
ianvernon approved these changes Oct 17, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@ianvernon ianvernon left a comment

Choose a reason for hiding this comment

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

😍😍😍😍

@aanm aanm force-pushed the pr/add-stoppable-wg branch from d50208d to 496668b Compare October 18, 2019 10:42
@ianvernon ianvernon added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Oct 18, 2019
@aanm aanm force-pushed the pr/add-stoppable-wg branch 2 times, most recently from c0c2ddd to 020e57c Compare October 21, 2019 09:26
@aanm aanm force-pushed the pr/refactor-k8s-watcher branch from 4a419ea to b8f94db Compare October 21, 2019 12:00
@aanm aanm changed the base branch from pr/add-stoppable-wg to master October 21, 2019 12:01
@aanm aanm removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Oct 21, 2019
@aanm
Copy link
Copy Markdown
Member Author

aanm commented Oct 21, 2019

test-me-please

@aanm
Copy link
Copy Markdown
Member Author

aanm commented Oct 21, 2019

unit tests were executed here https://github.com/cilium/cilium/pull/9468/checks?check_run_id=268448218

brb
brb approved these changes Oct 21, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@brb brb left a comment

Choose a reason for hiding this comment

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

LGTM, just one nit - I would use singular nouns for the watcher file names.

aanm added 3 commits October 21, 2019 17:59
@aanm
refactor daemon/k8s_watcher.go to pkg/k8s/watcher
1b06763 
Signed-off-by: André Martins <andre@cilium.io>
@aanm
k8s/watchers: refactor watchers to dedicated files
30e15b7 
Signed-off-by: André Martins <andre@cilium.io>
@aanm
k8s/watchers: re-order code and add comments
f562676 
Signed-off-by: André Martins <andre@cilium.io>
@aanm aanm force-pushed the pr/refactor-k8s-watcher branch from b8f94db to f562676 Compare October 21, 2019 16:06
@aanm
Copy link
Copy Markdown
Member Author

aanm commented Oct 21, 2019

test-me-please

@coveralls
Copy link
Copy Markdown

coveralls commented Oct 21, 2019

Coverage Status

Coverage decreased (-0.2%) to 45.367% when pulling f562676 on pr/refactor-k8s-watcher into c80801e on master.

@aanm aanm merged commit 0021474 into master Oct 21, 2019
@aanm aanm deleted the pr/refactor-k8s-watcher branch October 21, 2019 17:59
@joestringer joestringer added the release-note/misc This PR makes changes that have no direct user impact. label Nov 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brb brb brb approved these changes

+2 more reviewers

@houndci-bot houndci-bot houndci-bot left review comments

@ianvernon ianvernon ianvernon approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

kind/cleanup This includes no functional changes. release-note/misc This PR makes changes that have no direct user impact.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@aanm @coveralls @brb @ianvernon @houndci-bot @joestringer